[BUG] behaviour mismatch between ipv4 and ipv6 in UDP rx path

[BUG] behaviour mismatch between ipv4 and ipv6 in UDP rx path

[Chris Friesen]

Hi,

I sent this out a week ago but didn't see a reply, so I'm sending it out
again.

One of our guys is seeing occasional dropped ipv4 packets coming in on
an ipv6 udp socket obtained via socket(AF_INET6,  SOCK_DGRAM, IPPROTO_UDP).

Here's what he says:


"The problem happens when release_sock() goes down an interesting code
path.  If (sk->sk_backlog.tail) is non-NULL then release_sock() invokes
__release_sock() which loops over all queue packets and invokes the
socket's backlog receive function for each previously queued packet.

Now for the interesting part.  The UDPv6 backlog receive function (in
net/ipv6/udp.c, udpv6_queue_rcv_skb()) invokes xfrm6_policy_check() to
confirm that the packet is allowed, but the problem is that it calls
this function regardless of whether the packet is IPv4 or IPv6.  The
xfrm6_policy_check() function then assumes that it is an IPv6 packet and
tries to match a policy based on its packet header... but that clearly
won't work because the addresses that it finds when it decodes the skb
are completely bogus."


Looking at the ipv4 code, git commit 9382177 split __udp_queue_rcv_skb()
out of udp_queue_rcv_skb().  It was done for locking purposes, but it
also means that backlog_rcv is bound to __udp_queue_rcv_skb(), which
doesn't call xfrm4_policy_check().


Should a new function __udpv6_queue_rcv_skb() be split out from
udpv6_queue_rcv_skb() and bound to backlog_rcv to resolve the xfrm
issue?  What about the locking that was the reason for the split in the
ipv4 case--is there a similar problem with ipv6?

Thanks,
Chris

-- 
Chris Friesen
Software Developer
GENBAND
chris.friesen@genband.com
www.genband.com

Re: [BUG] behaviour mismatch between ipv4 and ipv6 in UDP rx path

[Eric Dumazet]

Le mercredi 16 février 2011 à 14:18 -0600, Chris Friesen a écrit :
> Hi,
> 
> I sent this out a week ago but didn't see a reply, so I'm sending it out
> again.
> 
> One of our guys is seeing occasional dropped ipv4 packets coming in on
> an ipv6 udp socket obtained via socket(AF_INET6,  SOCK_DGRAM, IPPROTO_UDP).
> 
> Here's what he says:
> 
> 
> "The problem happens when release_sock() goes down an interesting code
> path.  If (sk->sk_backlog.tail) is non-NULL then release_sock() invokes
> __release_sock() which loops over all queue packets and invokes the
> socket's backlog receive function for each previously queued packet.
> 
> Now for the interesting part.  The UDPv6 backlog receive function (in
> net/ipv6/udp.c, udpv6_queue_rcv_skb()) invokes xfrm6_policy_check() to
> confirm that the packet is allowed, but the problem is that it calls
> this function regardless of whether the packet is IPv4 or IPv6.  The
> xfrm6_policy_check() function then assumes that it is an IPv6 packet and
> tries to match a policy based on its packet header... but that clearly
> won't work because the addresses that it finds when it decodes the skb
> are completely bogus."
> 
> 
> Looking at the ipv4 code, git commit 9382177 split __udp_queue_rcv_skb()
> out of udp_queue_rcv_skb().  It was done for locking purposes, but it
> also means that backlog_rcv is bound to __udp_queue_rcv_skb(), which
> doesn't call xfrm4_policy_check().
> 
> 
> Should a new function __udpv6_queue_rcv_skb() be split out from
> udpv6_queue_rcv_skb() and bound to backlog_rcv to resolve the xfrm
> issue?  What about the locking that was the reason for the split in the
> ipv4 case--is there a similar problem with ipv6?
> 


Yes, please submit a patch ?

Ideally, __udp_queue_rcv_skb() should be the common .backlog

In practice, because of sock_rps_save_rxhash() and MIB counters, I
suspect a __udp6_queue_rcv_skb() is OK.