On 04/04/16(Mon) 13:09, David Gwynne wrote:
> this deprecates M_FILDROP.
>
> it is only set by bpf, and it is only respected on inbound packets.
> however, packets may be marked for dropping early, but it only comes
> into effect very late.
>
> this moves the dropping to right after the bpf calls. this is easy
> now that if_input run bpf on behalf of the driver, so only one place
> really needs changing.
>
> so if we remove the need for bpf to set flags on mbufs then we can
> say that bpf will not modify an mbuf at all and mark all the mbuf
> arguments as const.
>
> ok?
I like the idea a lot. I'd push the refactoring a bit further, see
below.
> Index: net/if.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if.c,v
> retrieving revision 1.429
> diff -u -p -r1.429 if.c
> --- net/if.c 16 Mar 2016 12:08:09 -0000 1.429
> +++ net/if.c 3 Apr 2016 23:41:57 -0000
> @@ -147,6 +147,7 @@ int if_group_egress_build(void);
>
> void if_watchdog_task(void *);
>
> +int if_input_filter(void *, const struct mbuf *);
> void if_input_process(void *);
>
> #ifdef DDB
> @@ -593,6 +594,12 @@ if_enqueue(struct ifnet *ifp, struct mbu
> struct mbuf_queue if_input_queue = MBUF_QUEUE_INITIALIZER(8192, IPL_NET);
> struct task if_input_task = TASK_INITIALIZER(if_input_process,
> &if_input_queue);
>
> +int
> +if_input_filter(void *if_bpf, const struct mbuf *m)
> +{
> + return bpf_mtap_ether(if_bpf, m, BPF_DIRECTION_IN);
> +}
> +
I find this filtering function confusing. What about introducing a
bpf_mtap() variant dealing with an ``mbuf_list'' : bpf_mltap()?
> void
> if_input(struct ifnet *ifp, struct mbuf_list *ml)
> {
> @@ -614,8 +621,15 @@ if_input(struct ifnet *ifp, struct mbuf_
> #if NBPFILTER > 0
> if_bpf = ifp->if_bpf;
> if (if_bpf) {
> - MBUF_LIST_FOREACH(ml, m)
> - bpf_mtap_ether(if_bpf, m, BPF_DIRECTION_IN);
> + struct mbuf *m0;
> +
> + m0 = ml_filter(ml, if_input_filter, if_bpf);
> + while (m0 != NULL) {
> + m = m0;
> + m0 = m->m_nextpkt;
> +
> + m_freem(m);
> + }
> }
> #endif
>
> Index: net80211/ieee80211_input.c
> ===================================================================
> RCS file: /cvs/src/sys/net80211/ieee80211_input.c,v
> retrieving revision 1.169
> diff -u -p -r1.169 ieee80211_input.c
> --- net80211/ieee80211_input.c 22 Mar 2016 11:37:35 -0000 1.169
> +++ net80211/ieee80211_input.c 3 Apr 2016 23:41:58 -0000
> @@ -546,16 +546,18 @@ ieee80211_input(struct ifnet *ifp, struc
> if (ifp->if_flags & IFF_DEBUG)
> ieee80211_input_print(ic, ifp, wh, rxi);
> #if NBPFILTER > 0
> - if (ic->ic_rawbpf)
> - bpf_mtap(ic->ic_rawbpf, m, BPF_DIRECTION_IN);
> - /*
> - * Drop mbuf if it was filtered by bpf. Normally, this is
> - * done in ether_input() but IEEE 802.11 management frames
> - * are a special case.
> - */
> - if (m->m_flags & M_FILDROP) {
> - m_freem(m);
> - return;
> + if (ic->ic_rawbpf) {
Instead of checking for NULL here, can't you simply return 0 inside
bpf_tap()?
> + if (bpf_mtap(ic->ic_rawbpf, m, BPF_DIRECTION_IN)) {
> + /*
> + * Drop mbuf if it was filtered by
> + * bpf. Normally, this is done in
> + * ether_input() but IEEE 802.11
> + * management frames are a special
> + * case.
> + */
> + m_freem(m);
> + return;
What about returning a mbuf (or a mbuf_list) instead of a boolean and
check for NULL? This would fit in the input_handler model where a
packet is "consumed", here by bpf(4), and processing should stop.
You could then write:
m = bpf_mtap(if->if_bpf, m, BPF_DIRECTION_IN);
if (m == NULL)
return;
