Re: [Intel-wired-lan] [PATCH iwl-net v1] i40e: don't advertise IFF_SUPP_NOFCS

[Kohei Enju <kohei@enjuk.jp>]

On 03/26 07:05, Paul Menzel wrote:
> Dear Kohei,
> 
> 
> Thank you for your patch.
> 
> Am 25.03.26 um 21:50 schrieb Kohei Enju:
> > i40e advertises IFF_SUPP_NOFCS, allowing users to use the SO_NOFCS
> > socket option. However, this option is silently ignored, as the driver
> > does not check skb->no_fcs, and always enables FCS insertion offload.
> > 
> > Fix this by removing the advertisement of IFF_SUPP_NOFCS.
> > 
> > This behavior can be reproduced with a simple AF_PACKET socket:
> > 
> >    import socket
> >    s = socket.socket(socket.AF_PACKET, socket.SOCK_RAW)
> >    s.setsockopt(socket.SOL_SOCKET, 43, 1) # SO_NOFCS
> >    s.bind(("eth0", 0))
> >    s.send(b'\xff' * 64)
> > 
> > Previously, send() succeeds but the driver ignores SO_NOFCS.
> 
> Would I check that with tcpdump?

On the RX side, it can be possible to observe such frames using tcpdump,
if the RX device supports:
  - rx-all (accepting all frames including bad frames)
  - keeping the FCS (not stripping it)

On the TX side, however, the hardware-generated FCS is not visible to
tcpdump.

In my setup, I verified that the CRC error stats didn't increase on the
RX side when using the script above. If SO_NOFCS were actually honored,
we would expect frames with invalid/custom CRC to be transmitted, and
the RX side device should report CRC errors.

> 
> > With this change, send() fails with -EPROTONOSUPPORT, as expected.
> 
> Great commit message! Thank you.
> 
> One question, did you look into, what is needed to implement working SO_NOFC
> support?

From the driver implementation, it looks like clearing
I40E_TX_DESC_CMD_ICRC when skb->no_fcs is set could enable the expected
behavior.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/intel/i40e/i40e_txrx.c#n3936

However, implementing proper support would require careful validation,
and also may impact the TX fast path slightly. So this patch focuses on
just correcting netdev->priv_flags.

> 
> > Fixes: 41c445ff0f48 ("i40e: main driver core")
> > Signed-off-by: Kohei Enju <kohei@enjuk.jp>
> > ---
> >   drivers/net/ethernet/intel/i40e/i40e_main.c | 1 -
> >   1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index 3749f32ef95a..a3c52aa6255b 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -13831,7 +13831,6 @@ static int i40e_config_netdev(struct i40e_vsi *vsi)
> >   	netdev->neigh_priv_len = sizeof(u32) * 4;
> >   	netdev->priv_flags |= IFF_UNICAST_FLT;
> > -	netdev->priv_flags |= IFF_SUPP_NOFCS;
> >   	/* Setup netdev TC information */
> >   	i40e_vsi_config_netdev_tc(vsi, vsi->tc_config.enabled_tc);
> 
> Nice catch.
> 
> 
> Kind regards,
> 
> Paul