Page MenuHomeFreeBSD

bpf: Add IfAPI analogue for bpf_peers_present()
ClosedPublic

Authored by jhibbits on Oct 4 2023, 8:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Nov 8, 3:06 PM
Unknown Object (File)
Mon, Oct 14, 4:35 PM
Unknown Object (File)
Oct 13 2024, 2:40 AM
Unknown Object (File)
Sep 27 2024, 2:47 PM
Unknown Object (File)
Sep 24 2024, 4:32 AM
Unknown Object (File)
Sep 23 2024, 6:34 PM
Unknown Object (File)
Sep 9 2024, 7:48 PM
Unknown Object (File)
Sep 9 2024, 7:48 PM

Details

Summary

An interface's bpf could feasibly not exist, in which case
bpf_peers_present() would panic from a NULL pointer dereference. Solve
this by adding a new IfAPI that includes a NULL check. Since this API
is used in only a handful of locations, it reduces the the NULL check
scope over inserting the check into bpf_peers_present().

Sponsored by: Juniper Networks, Inc.
MFC after: 1 week

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 53945
Build 50835: arc lint + arc unit

Event Timeline

zlei requested changes to this revision.Oct 10 2023, 3:22 AM
zlei added a subscriber: zlei.
zlei added inline comments.
sys/net/bpf.h
444

A quick grep shows that many drivers have bpf present.

# egrep -lr 'bpfattach' sys
sys/net/if_fwsubr.c
sys/net/if_ipsec.c
sys/net/if_loop.c
sys/net/if_ovpn.c
sys/net/if_disc.c
sys/net/if_gre.c
sys/net/if_tuntap.c
sys/net/if_ethersubr.c
sys/net/if_me.c
sys/net/if_gif.c
sys/net/if_enc.c
sys/net/if_stf.c
sys/net/if_infiniband.c
sys/netgraph/ng_iface.c
sys/net80211/ieee80211_radiotap.c
sys/dev/ppbus/if_plip.c
sys/dev/usb/net/if_usie.c
sys/dev/usb/net/uhso.c
sys/dev/usb/usb_pf.c
sys/dev/iicbus/if_ic.c
sys/dev/wg/if_wg.c
sys/netpfil/pf/if_pflog.c
sys/netpfil/pf/if_pfsync.c
sys/netpfil/ipfw/ip_fw_bpf.c

So I don't think it is a good idea to do NULL check everywhere.

This revision now requires changes to proceed.Oct 10 2023, 3:22 AM
sys/net/bpf.h
444

My alternative idea was to add a if_bpf_peers_present(if_t) (or bpf_peers_present_if()) IfAPI that does this work instead. Thoughts on that? Thoughts on which name, if so?

sys/net/bpf.h
444

My alternative idea was to add a if_bpf_peers_present(if_t) (or bpf_peers_present_if()) IfAPI that does this work instead. Thoughts on that? Thoughts on which name, if so?

Just keep naming consistent.

If it goes into sys/net/if.c then if_bpf_peers_present() looks good.
If it goes into sys/net/bpf.c then bpf_peers_present_if() sounds naturally.

Address @zlei's feedback and make an IfAPI.

jhibbits retitled this revision from bpf: Add seatbelt check for NULL bpf to bpf: Add IfAPI analogue for bpf_peers_present().Oct 12 2023, 2:42 PM
jhibbits edited the summary of this revision. (Show Details)
sys/dev/firewire/if_fwip.c
783

Firewire drivers call firewire_ifattach() to attach an interface. firewire_ifattach() will call bpfattach() to attach bpf_if.

sys/dev/hyperv/netvsc/if_hn.c
3265

if_hn is also an ethernet driver.

sys/dev/my/if_my.c
1155

if_my is ethernet. When ethernet drivers initialize, they calls ether_ifattach() which will call bpfattach(), so if_getbpf(ifp) will never return NULL.

Then a NULL check in bpf_peers_present_if() is redundant.

sys/dev/usb/usb_pf.c
410–411

if_getbpf(bus->ifp) == NULL

That seems not necessary.

usb_pf.c called bpfattach() to attach a struct bpf_if, so the ifp should always have a non-null bpf_if (it might be dead_bpf_if but it does not matter).

sys/net/bpf.c
2887

This NULL check is redundant. We always have non-NULL packet filter.
If that is not the case then it is a bug and should be fixed.

sys/net/bpf.c
2887

You're right. The NULL check is unnecessary in our stack. I'll remove it, because the whole purpose is to move this into the IfAPI, and Juniper's stack can do the needful.

Address further comments from @zlei.

Looks good to me.

sys/net/bpf.h
431

A cosmetic.
Since this is a new function, maybe we want bool instead of int ?

This revision is now accepted and ready to land.Oct 13 2023, 2:15 AM
sys/net/bpf.h
431

Good idea, I'll make the change before pushing.