bpf: Add IfAPI analogue for bpf_peers_present()
ClosedPublic
Actions

Authored by jhibbits on Oct 4 2023, 8:57 PM.

Details

Reviewers

glebius
bz
zlei

Group Reviewers

network

Commits

rG6c2430c72b4c: bpf: Add IfAPI analogue for bpf_peers_present()
rG8f31b879ecaf: bpf: Add IfAPI analogue for bpf_peers_present()

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 53839
Build 50730: arc lint + arc unit

Event Timeline

jhibbits created this revision.Oct 4 2023, 8:57 PM

Herald added subscribers: melifaro, ae, imp. · View Herald TranscriptOct 4 2023, 8:57 PM

jhibbits requested review of this revision.Oct 4 2023, 8:57 PM

Harbormaster completed remote builds in B53839: Diff 128263.Oct 4 2023, 8:58 PM

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

443

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

jhibbits added inline comments.Oct 10 2023, 2:43 PM

sys/net/bpf.h
443	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?

zlei added inline comments.Oct 11 2023, 7:58 AM

sys/net/bpf.h
443	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.

Harbormaster completed remote builds in B53940: Diff 128661.Oct 12 2023, 2:41 PM

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)

zlei added inline comments.Oct 12 2023, 3:46 PM

sys/dev/firewire/if_fwip.c
783 ↗	(On Diff #128661)	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 ↗	(On Diff #128661)	`if_hn` is also an ethernet driver.
sys/dev/my/if_my.c
1155 ↗	(On Diff #128661)	`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
411 ↗	(On Diff #128661)	`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 ↗	(On Diff #128661)	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.

jhibbits added inline comments.Oct 12 2023, 4:51 PM

sys/net/bpf.c
2887 ↗	(On Diff #128661)	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.