Re: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Joe Stringer]

On 18 April 2016 at 11:35, Pablo Neira Ayuso  wrote:
> On Thu, Apr 14, 2016 at 05:35:39PM -0700, Joe Stringer wrote:
>> On 14 April 2016 at 03:35, Pablo Neira Ayuso  wrote:
>> > On Thu, Apr 14, 2016 at 10:40:15AM +0200, Florian Westphal wrote:
>> >> David Laight  wrote:
>> >> > From: Joe Stringer
>> >> > > Sent: 13 April 2016 19:10
>> >> > > This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: 
>> >> > > Always
>> >> > > orphan skbs inside ip_defrag()").
>> >> > >
>> >> > > Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
>> >> > > clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would 
>> >> > > be
>> >> > > cloned (implicitly orphaning) prior to queueing for reassembly. As 
>> >> > > such,
>> >> > > when the IPv6 message is eventually reassembled, the skb->sk for all
>> >> > > fragments would be NULL. After that commit was introduced, rather than
>> >> > > cloning, the original skbs were queued directly without orphaning. The
>> >> > > end result is that all frags except for the first and last may have a
>> >> > > socket attached.
>> >> >
>> >> > I'd have thought that the queued fragments would still want to be
>> >> > resource-counted against the socket (I think that is what skb->sk is 
>> >> > for).
>> >>
>> >> No, ipv4/ipv6 reasm has its own accouting.
>> >>
>> >> > Although I can't imagine why IPv6 reassembly is happening on skb
>> >> > associated with a socket.
>> >>
>> >> Right, thats a much more interesting question -- both ipv4 and
>> >> ipv6 orphan skbs before NF_HOOK prerouting trip.
>> >>
>> >> (That being said, I don't mind the patch, I'm just be curious how this
>> >>  can happen).
>> >
>> > If this change is specific to get this working in ovs and its
>> > conntrack support, then I don't think this belong to core
>> > infrastructure. This should be fixed in ovs instead.
>>
>> I admit I've only been able to reproduce it with OVS. My main reason
>> for proposing the fix this way was just because this is what the IPv4
>> code does, so I figured IPv6 should be consistent with that.
>
> You mean that this is what you did in 029f7f3b8701 to fix this, right?
>
> But we shouldn't add code to the core that is OVS specific for no
> reason. We don't need this orphan from ipv4 and ipv6 as Florian
> indicated.

That makes complete sense to me. I was wondering whether the original
IPv4 fix was the correct one from the nf core perspective - but it
seems like perhaps it is, given that ip_defrag() has a lot more users
from a lot more different paths which are likely relying on the skb to
be orphaned. In comparison, in the IPv6 path, nf_ct_frag6_gather() is
only called from OVS and the netfilter hooks; the hooks already do the
orphaning, so it's more consistent for OVS to do it as well.

> Is there any chance you can fix this from OVS and its conntrack glue
> code? Thanks.

Sure, I'll resend the patch making the fix in OVS.

Re: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Pablo Neira Ayuso]

On Thu, Apr 14, 2016 at 05:35:39PM -0700, Joe Stringer wrote:
> On 14 April 2016 at 03:35, Pablo Neira Ayuso  wrote:
> > On Thu, Apr 14, 2016 at 10:40:15AM +0200, Florian Westphal wrote:
> >> David Laight  wrote:
> >> > From: Joe Stringer
> >> > > Sent: 13 April 2016 19:10
> >> > > This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
> >> > > orphan skbs inside ip_defrag()").
> >> > >
> >> > > Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
> >> > > clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would 
> >> > > be
> >> > > cloned (implicitly orphaning) prior to queueing for reassembly. As 
> >> > > such,
> >> > > when the IPv6 message is eventually reassembled, the skb->sk for all
> >> > > fragments would be NULL. After that commit was introduced, rather than
> >> > > cloning, the original skbs were queued directly without orphaning. The
> >> > > end result is that all frags except for the first and last may have a
> >> > > socket attached.
> >> >
> >> > I'd have thought that the queued fragments would still want to be
> >> > resource-counted against the socket (I think that is what skb->sk is 
> >> > for).
> >>
> >> No, ipv4/ipv6 reasm has its own accouting.
> >>
> >> > Although I can't imagine why IPv6 reassembly is happening on skb
> >> > associated with a socket.
> >>
> >> Right, thats a much more interesting question -- both ipv4 and
> >> ipv6 orphan skbs before NF_HOOK prerouting trip.
> >>
> >> (That being said, I don't mind the patch, I'm just be curious how this
> >>  can happen).
> >
> > If this change is specific to get this working in ovs and its
> > conntrack support, then I don't think this belong to core
> > infrastructure. This should be fixed in ovs instead.
> 
> I admit I've only been able to reproduce it with OVS. My main reason
> for proposing the fix this way was just because this is what the IPv4
> code does, so I figured IPv6 should be consistent with that.

You mean that this is what you did in 029f7f3b8701 to fix this, right?

But we shouldn't add code to the core that is OVS specific for no
reason. We don't need this orphan from ipv4 and ipv6 as Florian
indicated.

Is there any chance you can fix this from OVS and its conntrack glue
code? Thanks.

Re: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Joe Stringer]

On 14 April 2016 at 03:35, Pablo Neira Ayuso  wrote:
> On Thu, Apr 14, 2016 at 10:40:15AM +0200, Florian Westphal wrote:
>> David Laight  wrote:
>> > From: Joe Stringer
>> > > Sent: 13 April 2016 19:10
>> > > This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
>> > > orphan skbs inside ip_defrag()").
>> > >
>> > > Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
>> > > clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would be
>> > > cloned (implicitly orphaning) prior to queueing for reassembly. As such,
>> > > when the IPv6 message is eventually reassembled, the skb->sk for all
>> > > fragments would be NULL. After that commit was introduced, rather than
>> > > cloning, the original skbs were queued directly without orphaning. The
>> > > end result is that all frags except for the first and last may have a
>> > > socket attached.
>> >
>> > I'd have thought that the queued fragments would still want to be
>> > resource-counted against the socket (I think that is what skb->sk is for).
>>
>> No, ipv4/ipv6 reasm has its own accouting.
>>
>> > Although I can't imagine why IPv6 reassembly is happening on skb
>> > associated with a socket.
>>
>> Right, thats a much more interesting question -- both ipv4 and
>> ipv6 orphan skbs before NF_HOOK prerouting trip.
>>
>> (That being said, I don't mind the patch, I'm just be curious how this
>>  can happen).
>
> If this change is specific to get this working in ovs and its
> conntrack support, then I don't think this belong to core
> infrastructure. This should be fixed in ovs instead.

I admit I've only been able to reproduce it with OVS. My main reason
for proposing the fix this way was just because this is what the IPv4
code does, so I figured IPv6 should be consistent with that.

Re: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Joe Stringer]

On 14 April 2016 at 01:40, Florian Westphal  wrote:
> David Laight  wrote:
>> From: Joe Stringer
>> > Sent: 13 April 2016 19:10
>> > This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
>> > orphan skbs inside ip_defrag()").
>> >
>> > Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
>> > clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would be
>> > cloned (implicitly orphaning) prior to queueing for reassembly. As such,
>> > when the IPv6 message is eventually reassembled, the skb->sk for all
>> > fragments would be NULL. After that commit was introduced, rather than
>> > cloning, the original skbs were queued directly without orphaning. The
>> > end result is that all frags except for the first and last may have a
>> > socket attached.
>>
>> I'd have thought that the queued fragments would still want to be
>> resource-counted against the socket (I think that is what skb->sk is for).
>
> No, ipv4/ipv6 reasm has its own accouting.
>
>> Although I can't imagine why IPv6 reassembly is happening on skb
>> associated with a socket.
>
> Right, thats a much more interesting question -- both ipv4 and
> ipv6 orphan skbs before NF_HOOK prerouting trip.
>
> (That being said, I don't mind the patch, I'm just be curious how this
>  can happen).

The topology is quite simple, there is a veth pair connected between a
namespace and OVS. The OVS bridge has an internal port. The
bridge is configured with flows to send packets through conntrack
(causing packet reassembly + refragmentation on output), and then
forward packets between the host and the veth. The internal port and
the veth inside the netns have IP addresses configured in the same
subnet.

In the test case, we send a large ICMPv6 ping request from the
namespace to the internal port. The namespace will fragment the IP
message into fragments and send through the veth. OVS processes these,
sends to conntrack (reassembly), then decides to output to the
internal port (refragmenting). The host stack finally receives the
fragments and processes the ICMP request. On response, the host sends
several fragments to OVS. OVS reassembles these and sends them to
conntrack, then decides to forward to the veth. When forwarding to the
veth, the skb frag queue is in this state with many skbs (all except
first and last) having skb->sk populated, and we hit the
BUG_ON(skb->sk) in ip6_fragment() just prior to transmitting to the veth.

In regards to your question about prerouting, does the response even
hit the input path on the host? An ICMP response is generated, and it
needs to be directed out to the device (output path), then when the
internal device receives it, it starts OVS processing.

Re: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Pablo Neira Ayuso]

On Thu, Apr 14, 2016 at 10:40:15AM +0200, Florian Westphal wrote:
> David Laight  wrote:
> > From: Joe Stringer
> > > Sent: 13 April 2016 19:10
> > > This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
> > > orphan skbs inside ip_defrag()").
> > > 
> > > Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
> > > clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would be
> > > cloned (implicitly orphaning) prior to queueing for reassembly. As such,
> > > when the IPv6 message is eventually reassembled, the skb->sk for all
> > > fragments would be NULL. After that commit was introduced, rather than
> > > cloning, the original skbs were queued directly without orphaning. The
> > > end result is that all frags except for the first and last may have a
> > > socket attached.
> > 
> > I'd have thought that the queued fragments would still want to be
> > resource-counted against the socket (I think that is what skb->sk is for).
> 
> No, ipv4/ipv6 reasm has its own accouting.
> 
> > Although I can't imagine why IPv6 reassembly is happening on skb
> > associated with a socket.
> 
> Right, thats a much more interesting question -- both ipv4 and
> ipv6 orphan skbs before NF_HOOK prerouting trip.
> 
> (That being said, I don't mind the patch, I'm just be curious how this
>  can happen).

If this change is specific to get this working in ovs and its
conntrack support, then I don't think this belong to core
infrastructure. This should be fixed in ovs instead.

Re: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Florian Westphal]

David Laight  wrote:
> From: Joe Stringer
> > Sent: 13 April 2016 19:10
> > This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
> > orphan skbs inside ip_defrag()").
> > 
> > Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
> > clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would be
> > cloned (implicitly orphaning) prior to queueing for reassembly. As such,
> > when the IPv6 message is eventually reassembled, the skb->sk for all
> > fragments would be NULL. After that commit was introduced, rather than
> > cloning, the original skbs were queued directly without orphaning. The
> > end result is that all frags except for the first and last may have a
> > socket attached.
> 
> I'd have thought that the queued fragments would still want to be
> resource-counted against the socket (I think that is what skb->sk is for).

No, ipv4/ipv6 reasm has its own accouting.

> Although I can't imagine why IPv6 reassembly is happening on skb
> associated with a socket.

Right, thats a much more interesting question -- both ipv4 and
ipv6 orphan skbs before NF_HOOK prerouting trip.

(That being said, I don't mind the patch, I'm just be curious how this
 can happen).

RE: [PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[David Laight]

From: Joe Stringer
> Sent: 13 April 2016 19:10
> This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
> orphan skbs inside ip_defrag()").
> 
> Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
> clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would be
> cloned (implicitly orphaning) prior to queueing for reassembly. As such,
> when the IPv6 message is eventually reassembled, the skb->sk for all
> fragments would be NULL. After that commit was introduced, rather than
> cloning, the original skbs were queued directly without orphaning. The
> end result is that all frags except for the first and last may have a
> socket attached.

I'd have thought that the queued fragments would still want to be
resource-counted against the socket (I think that is what skb->sk is for).
So don't really want to orphan skb before queuing them, instead transfer
the entire resource count to the head skb when they are merged.

Although I can't imagine why IPv6 reassembly is happening on skb
associated with a socket.

David

[PATCH nf] netfilter: ipv6: Orphan skbs in nf_ct_frag6_gather()

[Joe Stringer]

This is the IPv6 equivalent of commit 8282f27449bf ("inet: frag: Always
orphan skbs inside ip_defrag()").

Prior to commit 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free
clone operations"), ipv6 fragments sent to nf_ct_frag6_gather() would be
cloned (implicitly orphaning) prior to queueing for reassembly. As such,
when the IPv6 message is eventually reassembled, the skb->sk for all
fragments would be NULL. After that commit was introduced, rather than
cloning, the original skbs were queued directly without orphaning. The
end result is that all frags except for the first and last may have a
socket attached.

This commit explicitly orphans such skbs during nf_ct_frag6_gather() to
prevent BUG_ON(skb->sk) during a later call to ip6_fragment().

kernel BUG at net/ipv6/ip6_output.c:631!
[...]
Call Trace:
 
 [] ? __lock_acquire+0x927/0x20a0
 [] ? do_output.isra.28+0x1b0/0x1b0 [openvswitch]
 [] ? __lock_is_held+0x52/0x70
 [] ovs_fragment+0x1f7/0x280 [openvswitch]
 [] ? mark_held_locks+0x75/0xa0
 [] ? _raw_spin_unlock_irqrestore+0x36/0x50
 [] ? dst_discard_out+0x20/0x20
 [] ? dst_ifdown+0x80/0x80
 [] do_output.isra.28+0xf3/0x1b0 [openvswitch]
 [] do_execute_actions+0x709/0x12c0 [openvswitch]
 [] ? ovs_flow_stats_update+0x74/0x1e0 [openvswitch]
 [] ? ovs_flow_stats_update+0xa1/0x1e0 [openvswitch]
 [] ? _raw_spin_unlock+0x27/0x40
 [] ovs_execute_actions+0x45/0x120 [openvswitch]
 [] ovs_dp_process_packet+0x85/0x150 [openvswitch]
 [] ? _raw_spin_unlock+0x27/0x40
 [] ovs_execute_actions+0xc4/0x120 [openvswitch]
 [] ovs_dp_process_packet+0x85/0x150 [openvswitch]
 [] ? key_extract+0x442/0xc10 [openvswitch]
 [] ovs_vport_receive+0x5d/0xb0 [openvswitch]
 [] ? __lock_acquire+0x927/0x20a0
 [] ? __lock_acquire+0x927/0x20a0
 [] ? __lock_acquire+0x927/0x20a0
 [] ? _raw_spin_unlock_irqrestore+0x36/0x50
 [] internal_dev_xmit+0x6d/0x150 [openvswitch]
 [] ? internal_dev_xmit+0x5/0x150 [openvswitch]
 [] dev_hard_start_xmit+0x2df/0x660
 [] ? validate_xmit_skb.isra.105.part.106+0x1a/0x2b0
 [] __dev_queue_xmit+0x8f5/0x950
 [] ? __dev_queue_xmit+0x50/0x950
 [] ? mark_held_locks+0x75/0xa0
 [] dev_queue_xmit+0x10/0x20
 [] neigh_resolve_output+0x178/0x220
 [] ? ip6_finish_output2+0x219/0x7b0
 [] ip6_finish_output2+0x219/0x7b0
 [] ? ip6_finish_output2+0x65/0x7b0
 [] ? ip_idents_reserve+0x6b/0x80
 [] ? ip6_fragment+0x93f/0xc50
 [] ip6_fragment+0xba1/0xc50
 [] ? ip6_flush_pending_frames+0x40/0x40
 [] ip6_finish_output+0xcb/0x1d0
 [] ip6_output+0x5f/0x1a0
 [] ? ip6_fragment+0xc50/0xc50
 [] ip6_local_out+0x3d/0x80
 [] ip6_send_skb+0x2f/0xc0
 [] ip6_push_pending_frames+0x4d/0x50
 [] icmpv6_push_pending_frames+0xac/0xe0
 [] icmpv6_echo_reply+0x42e/0x500
 [] icmpv6_rcv+0x4cf/0x580
 [] ip6_input_finish+0x1a7/0x690
 [] ? ip6_input_finish+0x5/0x690
 [] ip6_input+0x30/0xa0
 [] ? ip6_rcv_finish+0x1a0/0x1a0
 [] ip6_rcv_finish+0x4e/0x1a0
 [] ipv6_rcv+0x45f/0x7c0
 [] ? ipv6_rcv+0x36/0x7c0
 [] ? ip6_make_skb+0x1c0/0x1c0
 [] __netif_receive_skb_core+0x229/0xb80
 [] ? mark_held_locks+0x75/0xa0
 [] ? process_backlog+0x6f/0x230
 [] __netif_receive_skb+0x16/0x70
 [] process_backlog+0x78/0x230
 [] ? process_backlog+0xdd/0x230
 [] net_rx_action+0x203/0x480
 [] ? mark_held_locks+0x75/0xa0
 [] __do_softirq+0xde/0x49f
 [] ? ip6_finish_output2+0x228/0x7b0
 [] do_softirq_own_stack+0x1c/0x30
 
 [] do_softirq.part.18+0x3b/0x40
 [] __local_bh_enable_ip+0xb6/0xc0
 [] ip6_finish_output2+0x251/0x7b0
 [] ? ip6_fragment+0xba1/0xc50
 [] ? ip_idents_reserve+0x6b/0x80
 [] ? ip6_fragment+0x93f/0xc50
 [] ip6_fragment+0xba1/0xc50
 [] ? ip6_flush_pending_frames+0x40/0x40
 [] ip6_finish_output+0xcb/0x1d0
 [] ip6_output+0x5f/0x1a0
 [] ? ip6_fragment+0xc50/0xc50
 [] ip6_local_out+0x3d/0x80
 [] ip6_send_skb+0x2f/0xc0
 [] ip6_push_pending_frames+0x4d/0x50
 [] rawv6_sendmsg+0xa28/0xe30
 [] ? inet_sendmsg+0xc7/0x1d0
 [] inet_sendmsg+0x106/0x1d0
 [] ? inet_sendmsg+0x5/0x1d0
 [] sock_sendmsg+0x38/0x50
 [] SYSC_sendto+0xf6/0x170
 [] ? trace_hardirqs_on_thunk+0x1b/0x1d
 [] SyS_sendto+0xe/0x10
 [] entry_SYSCALL_64_fastpath+0x18/0xa8
Code: 06 48 83 3f 00 75 26 48 8b 87 d8 00 00 00 2b 87 d0 00 00 00 48 39 d0 72 
14 8b 87 e4 00 00 00 83 f8 01 75 09 48 83 7f 18 00 74 9a <0f> 0b 41 8b 86 cc 00 
00 00 49 8#
RIP  [] ip6_fragment+0x73a/0xc50
 RSP 

Fixes: 029f7f3b8701 ("netfilter: ipv6: nf_defrag: avoid/free clone
operations")
Reported-by: Daniele Di Proietto 
Signed-off-by: Joe Stringer 
---
 net/ipv6/netfilter/nf_conntrack_reasm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c 
b/net/ipv6/netfilter/nf_conntrack_reasm.c
index e4347aeb2e65..fd2fda0ae75a 100644
--- a/net/ipv6/netfilter/nf_conntrack_reasm.c
+++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
@@ -595,6 +595,7 @@ int nf_ct_frag6_gather(struct net *net, struct sk_buff 
*skb, u32 user)
pr_debug("Can't find and can't create new queue\n");
return -ENOMEM;
}
+   skb_orphan(skb);
 
spin_lock_bh(>q.lock);
 
--