skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Ian Campbell]

Hi,

We are seeing cases where Xen netback's start_xmit is being passed an
skb which has a ->frag_list, despite the driver not advertising the
NETIF_F_FRAGLIST feature.

Is this indicative of a problem somewhere? Are drivers expected to
handle a frag_list? grepping for frag_list in drivers/net it appears not
many drivers do anything with the frag_list.

The netback driver is bridged with a tg3 physical device and we think
the problematic skb's are originating on the wire.

The case we are actually seeing is with 2.6.32 + tg3 3.110g so obviously
the next step is to reproduce on a more modern kernel and the in-tree
driver and then to attempt to determine if the fault is in what the
physical interface's driver is passing up the stack or in the stack's
handling of those skbs. Any hints on where to look would be much
appreciated.

In particular I'm not sure where the frag_list is supposed to get
resolved in the case where dev_hard_start_xmit takes the netif_needs_gso
== true path and calls dev_gso_segment rather than taking the
__skb_linearize path (the issue appears to occur only when the netback
device has NETIF_F_GSO but not NETIF_F_TSO). AFAICT dev_gso_segment goes
to tcp_tso_segment and then to skb_segment which does appear to create
skbs with a frag_list (although it's not outside the realms of
possibility that I'm reading it wrong).

I did wonder if perhaps we should just ignore the field, since something
higher up the stack walked the list and called start_xmit repeatedly,
but if that's the case I can't see where...

Ian.

Re: skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Jesse Gross]

On Tue, Mar 1, 2011 at 3:46 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> Hi,
>
> We are seeing cases where Xen netback's start_xmit is being passed an
> skb which has a ->frag_list, despite the driver not advertising the
> NETIF_F_FRAGLIST feature.
>
> Is this indicative of a problem somewhere? Are drivers expected to
> handle a frag_list? grepping for frag_list in drivers/net it appears not
> many drivers do anything with the frag_list.

It certainly sounds like a problem to me.  Off the top of my head I
don't know of any drivers that actually set and handle
NETIF_F_FRAGLIST (except for pseudo-devices like bridging).

>
> The netback driver is bridged with a tg3 physical device and we think
> the problematic skb's are originating on the wire.
>
> The case we are actually seeing is with 2.6.32 + tg3 3.110g so obviously
> the next step is to reproduce on a more modern kernel and the in-tree
> driver and then to attempt to determine if the fault is in what the
> physical interface's driver is passing up the stack or in the stack's
> handling of those skbs. Any hints on where to look would be much
> appreciated.

I'd guess that the most likely source of frag_lists generated here are
from GRO, in skb_gro_receive().  The driver/NIC can definitely
influence the strategy that GRO uses for reassembly but it seems less
likely that the driver itself will create frag_lists.

>
> In particular I'm not sure where the frag_list is supposed to get
> resolved in the case where dev_hard_start_xmit takes the netif_needs_gso
> == true path and calls dev_gso_segment rather than taking the
> __skb_linearize path (the issue appears to occur only when the netback
> device has NETIF_F_GSO but not NETIF_F_TSO). AFAICT dev_gso_segment goes
> to tcp_tso_segment and then to skb_segment which does appear to create
> skbs with a frag_list (although it's not outside the realms of
> possibility that I'm reading it wrong).

I'm fairly certain that the problem is in skb_segment().  It's not the
most tolerant of skbs with frag_lists that do not line up with the
requested mss.  Depending on how the original skb is laid out,
sometimes this will trigger a BUG_ON and sometimes it creates new
frag_lists.  Since there are no further checks after GSO, the skb with
a frag_list will get passed to the driver, even if it is not
supported.

I believe that not much has changed in this regard between 2.6.32 and net-next.

> I did wonder if perhaps we should just ignore the field, since something
> higher up the stack walked the list and called start_xmit repeatedly,
> but if that's the case I can't see where...

That will happen for GSO but those packets shouldn't have a frag_list
if the driver doesn't support it.

Re: skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Ian Campbell]

Hi Jesse,

On Tue, 2011-03-01 at 23:41 +0000, Jesse Gross wrote:
> On Tue, Mar 1, 2011 at 3:46 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > Hi,
> >
> > We are seeing cases where Xen netback's start_xmit is being passed an
> > skb which has a ->frag_list, despite the driver not advertising the
> > NETIF_F_FRAGLIST feature.
> >
> > Is this indicative of a problem somewhere? Are drivers expected to
> > handle a frag_list? grepping for frag_list in drivers/net it appears not
> > many drivers do anything with the frag_list.
> 
> It certainly sounds like a problem to me.  Off the top of my head I
> don't know of any drivers that actually set and handle
> NETIF_F_FRAGLIST (except for pseudo-devices like bridging).

That's basically all I found with grep too.

> > The netback driver is bridged with a tg3 physical device and we think
> > the problematic skb's are originating on the wire.
> >
> > The case we are actually seeing is with 2.6.32 + tg3 3.110g so obviously
> > the next step is to reproduce on a more modern kernel and the in-tree
> > driver and then to attempt to determine if the fault is in what the
> > physical interface's driver is passing up the stack or in the stack's
> > handling of those skbs. Any hints on where to look would be much
> > appreciated.
> 
> I'd guess that the most likely source of frag_lists generated here are
> from GRO, in skb_gro_receive().  The driver/NIC can definitely
> influence the strategy that GRO uses for reassembly but it seems less
> likely that the driver itself will create frag_lists.

Agreed, I cant see anywhere in the driver which would do this and
skb_gro_receive does seem like the likely source of the frag list.

> > In particular I'm not sure where the frag_list is supposed to get
> > resolved in the case where dev_hard_start_xmit takes the netif_needs_gso
> > == true path and calls dev_gso_segment rather than taking the
> > __skb_linearize path (the issue appears to occur only when the netback
> > device has NETIF_F_GSO but not NETIF_F_TSO). AFAICT dev_gso_segment goes
> > to tcp_tso_segment and then to skb_segment which does appear to create
> > skbs with a frag_list (although it's not outside the realms of
> > possibility that I'm reading it wrong).
> 
> I'm fairly certain that the problem is in skb_segment().  It's not the
> most tolerant of skbs with frag_lists that do not line up with the
> requested mss.  Depending on how the original skb is laid out,
> sometimes this will trigger a BUG_ON and sometimes it creates new
> frag_lists.  Since there are no further checks after GSO, the skb with
> a frag_list will get passed to the driver, even if it is not
> supported.

I wondered if that was the case, but skb_segment made my head spin a
bit ;-)

Do you think there might be a case for adding a some skb_needs_linearize
and __skb_linearize calls on the netif_needs_gso paths too, perhaps with
some WARN_ON's?

The first check in skb_needs_linearize looks at skb->data_len, I thought
skb->data_len only covered the paged fragments but the conditions in
skb_needs_linearize seem to imply that it covers the frag_list length
too? Was I simply mistaken?

FWIW an example of the sort of thing we see is:

skb: len=0x59a, data_len=0x55a, headlen=64 (matches tail-data), no paged
frags

skb->frag_list: len=0x55a, data_len=0, headlen=0x55a (matches
tail-data), no paged frags

> I believe that not much has changed in this regard between 2.6.32 and net-next.

It turns out I cannot reproduce with either 2.6.32 (pvops xen.git) or a
more recent ((26.38-rc) kernel. The kernel where we see this is the XCP
kernel which is a 2.6.32 based thing derived from SLES11SP1 + XCP
specific updates.

I looked through the diff of net/core and net/ipv4 between a reproducing
and non-reproducing kernel and didn't see anything glaringly obvious.

Anyway, since I can't reproduce on a mainline kernel I'll stop bothering
you all with it.

Ian.

Re: skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Ian Campbell]

On Wed, 2011-03-02 at 13:12 +0000, Ian Campbell wrote:
> 
> > I believe that not much has changed in this regard between 2.6.32
> and net-next.
> 
> It turns out I cannot reproduce with either 2.6.32 (pvops xen.git) or
> a more recent ((26.38-rc) kernel. The kernel where we see this is the
> XCP kernel which is a 2.6.32 based thing derived from SLES11SP1 + XCP
> specific updates.

Paul reminded me that his XCP system where we see this is running
openvswitch not bridging (all my own tests were with bridging), is this
something which could be caused by vswitch at all? I don't see any
interesting ->frag_list usage in openvswitch.git.

Anyway I'm going to see if I can repro with vswitch.

Ian.

Re: skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Ian Campbell]

On Wed, 2011-03-02 at 13:14 +0000, Ian Campbell wrote:
> On Wed, 2011-03-02 at 13:12 +0000, Ian Campbell wrote:
> > 
> > > I believe that not much has changed in this regard between 2.6.32
> > and net-next.
> > 
> > It turns out I cannot reproduce with either 2.6.32 (pvops xen.git) or
> > a more recent ((26.38-rc) kernel. The kernel where we see this is the
> > XCP kernel which is a 2.6.32 based thing derived from SLES11SP1 + XCP
> > specific updates.
> 
> Paul reminded me that his XCP system where we see this is running
> openvswitch not bridging (all my own tests were with bridging), is this
> something which could be caused by vswitch at all? I don't see any
> interesting ->frag_list usage in openvswitch.git.
> 
> Anyway I'm going to see if I can repro with vswitch.

It seems that was the key.

Using openvswitch.git aae3743bf24cd0e14be726c774a0be49ff0459d7 on top of
2.6.38-rc7 (+ some Xen stuff from linux-next and my netback branch). I
added:
	WARN_ONCE(skb_shinfo(skb)->frag_list, "skb %p has a frag list\n", skb)
to netback's start_xmit and saw it trigger (full trace below).

I've added ovs-dev, you guys can see the complete thread at
http://thread.gmane.org/gmane.linux.network/187720 . The gist is that we
are seeing skb's passed to a driver's start_xmit which have a
->frag_list despite not advertising NETIF_F_FRAGLIST. The netback device
has GSO but not TSO enabled.

(aae3743bf24cd0e14be726c774a0be49ff0459d7 is from the end of January but
it does appear to be the current master, I guess it's a recently rebased
+pushed old commit).

Cheers,
Ian.

------------[ cut here ]------------
WARNING: at /local/scratch/ianc/devel/kernels/linux-2.6/drivers/net/xen-netback/interface.c:84 xenvif_start_xmit+0x109/0x120()
Hardware name: PowerEdge 860
skb cfa14c80 has a frag list
Modules linked in: openvswitch_mod
Pid: 0, comm: swapper Not tainted 2.6.38-rc7-x86_32p-xen0-00190-g0716c28-dirty #375
Call Trace:
 [<c12bfbf9>] ? xenvif_start_xmit+0x109/0x120
 [<c103f17c>] ? warn_slowpath_common+0x6c/0xa0
 [<c12bfbf9>] ? xenvif_start_xmit+0x109/0x120
 [<c103f22e>] ? warn_slowpath_fmt+0x2e/0x30
 [<c12bfbf9>] ? xenvif_start_xmit+0x109/0x120
 [<c13500d4>] ? dev_hard_start_xmit+0x2c4/0x5a0
 [<c134fe58>] ? dev_hard_start_xmit+0x48/0x5a0
 [<c13639ec>] ? sch_direct_xmit+0x12c/0x230
 [<c142be5a>] ? _raw_spin_lock+0x3a/0x40
 [<c13505de>] ? dev_queue_xmit+0x22e/0x7a0
 [<c13503b0>] ? dev_queue_xmit+0x0/0x7a0
 [<f8c77ab7>] ? netdev_send+0x17/0x20 [openvswitch_mod]
 [<f8c755a0>] ? vport_send+0x40/0x100 [openvswitch_mod]
 [<c100790a>] ? xen_force_evtchn_callback+0x1a/0x30
 [<c10080f0>] ? xen_restore_fl_direct+0x0/0x17
 [<f8c6d70b>] ? execute_actions+0x62b/0x7f0 [openvswitch_mod]
 [<c106b0d2>] ? mark_held_locks+0x62/0x80
 [<c106b3f2>] ? trace_hardirqs_on_caller+0xa2/0x1f0
 [<f8c726b3>] ? flow_used+0x63/0x90 [openvswitch_mod]
 [<c1044fb4>] ? local_bh_enable_ip+0xa4/0x110
 [<c142c5b5>] ? _raw_spin_unlock_bh+0x25/0x30
 [<f8c726b3>] ? flow_used+0x63/0x90 [openvswitch_mod]
 [<f8c6f0aa>] ? dp_process_received_packet+0x8a/0x200 [openvswitch_mod]
 [<c10450b6>] ? local_bh_enable+0x96/0x110
 [<c106b3f2>] ? trace_hardirqs_on_caller+0xa2/0x1f0
 [<f8c756ec>] ? vport_receive+0x8c/0xa0 [openvswitch_mod]
 [<f8c756a7>] ? vport_receive+0x47/0xa0 [openvswitch_mod]
 [<f8c77bcf>] ? netdev_frame_hook+0xaf/0xe0 [openvswitch_mod]
 [<c134dff5>] ? __netif_receive_skb+0x185/0x340
 [<c134dee2>] ? __netif_receive_skb+0x72/0x340
 [<c1008114>] ? check_events+0x8/0xc
 [<f8c77b20>] ? netdev_frame_hook+0x0/0xe0 [openvswitch_mod]
 [<c134edbb>] ? netif_receive_skb+0xcb/0xe0
 [<c134ed0a>] ? netif_receive_skb+0x1a/0xe0
 [<c134ef0f>] ? napi_gro_complete+0x3f/0x110
 [<c134ef18>] ? napi_gro_complete+0x48/0x110
 [<c134f140>] ? dev_gro_receive+0x160/0x280
 [<c134f057>] ? dev_gro_receive+0x77/0x280
 [<c134f41b>] ? napi_gro_receive+0xcb/0xe0
 [<c12aae75>] ? tg3_poll_work+0x5e5/0xda0
 [<c106b0d2>] ? mark_held_locks+0x62/0x80
 [<c100790a>] ? xen_force_evtchn_callback+0x1a/0x30
 [<c100790a>] ? xen_force_evtchn_callback+0x1a/0x30
 [<c10080f0>] ? xen_restore_fl_direct+0x0/0x17
 [<c1008114>] ? check_events+0x8/0xc
 [<c100810b>] ? xen_restore_fl_direct_end+0x0/0x1
 [<c100790a>] ? xen_force_evtchn_callback+0x1a/0x30
 [<c10080f0>] ? xen_restore_fl_direct+0x0/0x17
 [<c11d08f0>] ? xen_swiotlb_unmap_page+0x0/0x20
 [<c142c5f1>] ? _raw_spin_unlock_irq+0x31/0x40
 [<c12ab714>] ? tg3_poll+0x44/0x1d0
 [<c134f5fc>] ? net_rx_action+0xcc/0x1a0
 [<c134f5fc>] ? net_rx_action+0xcc/0x1a0
 [<c1044dbf>] ? __do_softirq+0x9f/0x170
 [<c1044d20>] ? __do_softirq+0x0/0x170
 <IRQ>  [<c1003640>] ? xen_cpuid+0x0/0xa0
 [<c1044b45>] ? irq_exit+0x65/0x90
 [<c11c5800>] ? xen_evtchn_do_upcall+0x20/0x30
 [<c100bc2f>] ? xen_do_upcall+0x7/0xc
 [<c1003640>] ? xen_cpuid+0x0/0xa0
 [<c10023a7>] ? hypercall_page+0x3a7/0x1010
 [<c10079f2>] ? xen_safe_halt+0x12/0x30
 [<c1003640>] ? xen_cpuid+0x0/0xa0
 [<c1012d30>] ? default_idle+0x40/0xa0
 [<c100a769>] ? cpu_idle+0x59/0xa0
 [<c1408d71>] ? rest_init+0xa1/0xb0
 [<c1408cd0>] ? rest_init+0x0/0xb0
 [<c10080e0>] ? xen_save_fl_direct+0x0/0xd
 [<c1613a25>] ? start_kernel+0x37c/0x399
 [<c1613467>] ? unknown_bootoption+0x0/0x1f1
 [<c16130b7>] ? i386_start_kernel+0xa6/0xe2
 [<c161676a>] ? xen_start_kernel+0x5fc/0x6b2
---[ end trace 877717720328b880 ]---

Re: skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Jesse Gross]

On Wed, Mar 2, 2011 at 9:42 AM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2011-03-02 at 13:14 +0000, Ian Campbell wrote:
>> On Wed, 2011-03-02 at 13:12 +0000, Ian Campbell wrote:
>> >
>> > > I believe that not much has changed in this regard between 2.6.32
>> > and net-next.
>> >
>> > It turns out I cannot reproduce with either 2.6.32 (pvops xen.git) or
>> > a more recent ((26.38-rc) kernel. The kernel where we see this is the
>> > XCP kernel which is a 2.6.32 based thing derived from SLES11SP1 + XCP
>> > specific updates.
>>
>> Paul reminded me that his XCP system where we see this is running
>> openvswitch not bridging (all my own tests were with bridging), is this
>> something which could be caused by vswitch at all? I don't see any
>> interesting ->frag_list usage in openvswitch.git.
>>
>> Anyway I'm going to see if I can repro with vswitch.
>
> It seems that was the key.
>
> Using openvswitch.git aae3743bf24cd0e14be726c774a0be49ff0459d7 on top of
> 2.6.38-rc7 (+ some Xen stuff from linux-next and my netback branch). I
> added:
>        WARN_ONCE(skb_shinfo(skb)->frag_list, "skb %p has a frag list\n", skb)
> to netback's start_xmit and saw it trigger (full trace below).

Open vSwitch shouldn't create or use frag_lists in the normal packet
processing path.  However, it does pull more data into the linear data
area compared to the Linux bridge, which affects the skb geometry.  In
fact, looking at the lengths that you previously sent, the linear data
area contains 64 bytes, which is the default amount that we pull in.
It sounds like the original packet that was received was small, GRO
attached a larger one to it with a frag_list, and Open vSwitch pulled
data across, screwing up the layout.  It's a little surprising that
the first packet would be so small but it's possible.

Regardless, it seems like this is a valid operation and something that
skb_segment() should be able to handle.

Re: skb->frag_list != NULL in start_xmit for device without NETIF_F_FRAGLIST

[Jesse Gross]

On Wed, Mar 2, 2011 at 5:12 AM, Ian Campbell <Ian.Campbell@eu.citrix.com> wrote:
>> > In particular I'm not sure where the frag_list is supposed to get
>> > resolved in the case where dev_hard_start_xmit takes the netif_needs_gso
>> > == true path and calls dev_gso_segment rather than taking the
>> > __skb_linearize path (the issue appears to occur only when the netback
>> > device has NETIF_F_GSO but not NETIF_F_TSO). AFAICT dev_gso_segment goes
>> > to tcp_tso_segment and then to skb_segment which does appear to create
>> > skbs with a frag_list (although it's not outside the realms of
>> > possibility that I'm reading it wrong).
>>
>> I'm fairly certain that the problem is in skb_segment().  It's not the
>> most tolerant of skbs with frag_lists that do not line up with the
>> requested mss.  Depending on how the original skb is laid out,
>> sometimes this will trigger a BUG_ON and sometimes it creates new
>> frag_lists.  Since there are no further checks after GSO, the skb with
>> a frag_list will get passed to the driver, even if it is not
>> supported.
>
> I wondered if that was the case, but skb_segment made my head spin a
> bit ;-)
>
> Do you think there might be a case for adding a some skb_needs_linearize
> and __skb_linearize calls on the netif_needs_gso paths too, perhaps with
> some WARN_ON's?

Definitely something needs to be done here, as drivers clearly should
not be handed skbs with features that they don't support.  I'm not
sure what the best way to do that is though.  I would like it if
skb_segment() could be made more tolerant of frag_lists in general,
including the cases that currently have BUG_ON.

Currently the function tries to generate packets that are exactly MSS
size (except for the last one).  If the frag_lists don't align with
this we either get BUG or a new skb with a frag_list.  However, the
MSS is simply a maximum - it's also valid to generate packets smaller
than it.  If we did that we would probably be able to handle all of
these cases without doing any linearization.  It's possible that in
some cases it would be faster to do the linearization instead of
generating additional packets but on the other hand the situations
that this affects aren't handled correctly at all right now, so they
probably don't come up too often.

>
> The first check in skb_needs_linearize looks at skb->data_len, I thought
> skb->data_len only covered the paged fragments but the conditions in
> skb_needs_linearize seem to imply that it covers the frag_list length
> too? Was I simply mistaken?

skb->data_len includes both the paged fragments and frag_list.