> On 4 Apr 2016, at 6:41 PM, Martin Pieuchot <[email protected]> wrote:
>
> 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()?
i could, but passing the direction through is difficult. i could have different
wrappers for different directions though.
>
>> 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()?
oh yeah, we do that now. ill tweak.
>
>> + 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;
i wouldnt be able to pass the mbuf as a const struct mbuf * and return it
without casting it to drop the const qualifier. to me it feels more like its
being tested than consumed.
dlg
