Re: [RFC PATCH net] net-gro: restore check for NULL skb in napi_gro_frags

[Edward Cree <ecree.xilinx@gmail.com>]

On 02/08/2023 11:22, Eric Dumazet wrote:
> On Wed, Aug 2, 2023 at 11:42 AM <edward.cree@amd.com> wrote:
>> An sfc customer has encountered this panic in the wild; we're still
>>  investigating exactly how it happened (we have a reproducer) but it
>>  seems wise to have the core handle this check rather than requiring
>>  it in every driver.
> 
> An ethernet driver feeding non-ethernet packets to the upper stacks
> seems weird to me,
...
> Not sure why a napi_gro_frags() enabled driver would be allowed to
> cook arbitrary packets with length <  ETH_HLEN
...
> Mixed feelings here
Fwiw we now have some more understanding of what caused this: the
 device produced an RX descriptor with a zero in its buffer length
 field (there was actually data in the buffer, but a — we think —
 firmware bug caused the length to be stored in the wrong place).
And the driver, blithely trusting the device, attached the RX page
 to the skb from napi_get_frags, passing the buffer length it had
 straight to skb_fill_page_desc without checking it.  (After all,
 the device had told us through RX event flags that the packet was
 TCP, so it had to have seen a complete set of headers.)
This certainly can be fixed in the driver, by adding a length
 check before calling napi_gro_frags(), but I see two reasons to
 put it in the core instead.
1) The same issue is likely to be present in any napi_gro_frags()
   driver; I've just looked at e1000 and mlx4 (as examples) and it
   looks like they could both be susceptible if hw misbehaved in a
   similar way.
2) The core can recycle the SKB, but drivers can't because
   napi_reuse_skb is static.  Instead, if they've already called
   napi_get_frags when they discover the problem, they have to do
	kfree_skb(skb);
	napi->skb = NULL;
   which is not pretty.  Of course we could always export
   napi_reuse_skb...
Another open question is whether, if we do put this in the core,
 we should do the same for the other RX entry points
 (napi_gro_receive, eth_type_trans) that could see something
 similar.

Anyway, I'll post v2, with unlikely() per Tyler's analysis, and you
 can ack or nack as the mood takes you.

-ed