Message-ID: <CAF2d9jgZ+32LT5FxcqH97n1rbEChEgK7a4RtCEiUaUWOEyTmhA@mail.gmail.com>
Date:	Thu, 9 Apr 2015 09:33:59 -0700
From:	Mahesh Bandewar <maheshb@...gle.com>
To:	Dan Williams <dcbw@...hat.com>
Cc:	linux-netdev <netdev@...r.kernel.org>, jbenc@...hat.com
Subject: Re: [PATCH] ipvlan: fix up broadcast MAC filtering for ARP and DHCP

>> You're right.  I mis-read your proposal above.
>>
>> But having re-read it, are you proposing a 2m timer on interface up?  If
>> so (and ignore this if not) I don't think that works well either,
>> because there's no guarantee that interface configuration will happen
>> only close to interface up.  Maybe userspace adds IPv6 addresses
>> initially, and then later tries to do DHCP for some reason.  We simply
>> cannot rely on specific ordering of operations that userspace might want
>> to do.
>>
>> If you don't mean a 2m timer from interface up, then ignore that, and
>> then what kind of time do you mean? :)
>>
>> > If the link is stripped off of address(es), then it should again go
>> > back into the config-mode where it would turn on the broadcast bit and
>> > enable timer to get it configured. May be I'm missing something.
>>
>> As above, I was wrong about the DHCPv4 lease expiration thing, but this
>> may still run afoul of userspace operation ordering if you are talking
>> about a 2m timer from interface up.
>>
>> Just had another thought though; what if instead of snooping for all the
>> DHCP stuff, the code just snooped outgoing IPv4 packets for a broadcast
>> destination address?  Then turn on the broadcast bit filter for 2m.
>> That would look something like this:
>>
>> static bool is_bcast4(struct sk_buff *skb)
>> {
>>       struct iphdr *ip4h;
>>
>>       switch (skb->protocol) {
>>       case htons(ETH_P_ALL):
>>               /* Raw sockets */
>>               if (eth_hdr(skb)->h_proto != htons(ETH_P_IP))
>>                       break;
>>               /* Fall through */
>>       case htons(ETH_P_IP):
>>               if (unlikely(!pskb_may_pull(skb, sizeof(*ip4h))))
>>                       return NULL;
>>               ip4h = ip_hdr(skb);
>>               if (ip4h->ihl < 5 || ip4h->version != 4)
>>                       return NULL;
>>               return ip4h->daddr == INADDR_BROADCAST;
>>       }
>>       return false;
>> }
>>
>> static int ipvlan_xmit_mode_l2(...)
>> {
>>       if (!ipvlan->ipv4cnt && !ipvlan->bcast4_seen && is_bcast4(skb)) {
>>               ipvlan->bcast4_seen = true;
>>               ipvlan_set_broadcast_mac_filter(ipvlan, true);
>>       }
>>
>> Yes, it's still snooping for all packets, but it's a lot fewer compares
>> than looking for DHCP specifically.
>
> Any thoughts on this Mahesh?  Is this non-DHCP approach more to your
> liking?  If so I'll generate an actual patch and do some testing.
>
Sorry about the late reply but some how I missed that.

Yes, this looks better, but I'm thinking of solving this issue with a
different approach which does not involve snooping. Or if it does, it
wont be in a fast path! It is sort of falling back to your original
patch which eliminates setting / resetting the broadcast bit but after
deferring the broadcast / multicast processing to a work-queue. This
will keep the fast path clean and we can do all sort of jugglery in
work-queue (if needed) without affecting the performance of the device
(fast path) whether it's IPv6 or IPv4 traffic.

Also Eric pointed out how multicast is broken in IPvlan, that needs a
fix too. I have cooked something, but needs some testing, I'll push
out those patches as soon as I'm happy with it's testing.

Thanks,
--mahesh..


> Dan
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists