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 <[email protected]>
> > ---
> > 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
