| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<BY3PR18MB4737A4F337BE3417177090B7C62C2@BY3PR18MB4737.namprd18.prod.outlook.com>
Date: Tue, 19 Mar 2024 10:26:52 +0000
From: Sunil Kovvuri Goutham <sgoutham@...vell.com>
To: Simon Horman <horms@...nel.org>, Erwan Velu <erwanaliasr1@...il.com>
CC: Erwan Velu <e.velu@...teo.com>,
Jesse Brandeburg
<jesse.brandeburg@...el.com>,
Tony Nguyen <anthony.l.nguyen@...el.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS
> -----Original Message-----
> From: Simon Horman <horms@...nel.org>
> Sent: Monday, March 18, 2024 11:15 PM
> To: Erwan Velu <erwanaliasr1@...il.com>
> Cc: Erwan Velu <e.velu@...teo.com>; Jesse Brandeburg
> <jesse.brandeburg@...el.com>; Tony Nguyen
> <anthony.l.nguyen@...el.com>; David S. Miller <davem@...emloft.net>;
> Eric Dumazet <edumazet@...gle.com>; Jakub Kicinski <kuba@...nel.org>;
> Paolo Abeni <pabeni@...hat.com>; intel-wired-lan@...ts.osuosl.org;
> netdev@...r.kernel.org; linux-kernel@...r.kernel.org
> Subject: [EXTERNAL] Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if
> greater than MFS
>
> On Wed, Mar 13, 2024 at 10:07:16AM +0100, Erwan Velu wrote:
> > Commit 6871a7de705 ("[intelxl] Use admin queue to set port MAC
> address
> > and maximum frame size") from iPXE project set the MFS to 0x600 =
> 1536.
> > See
> > https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_ipxe_i
> >
> pxe_commit_6871a7de705&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=q
> 3VKxXQKibo
> >
> Rw_F01ggTzHuhwawxR1P9_tMCN2FODU4&m=J_D0216agwnkMc8GuVR13
> qCCDBQWZmqULkA
> >
> mTpcwoDk9e1Yw0Z28r7RoYVuLyMex&s=s9O0JKM9qId17Qw5d2dH7c3wLJT
> Bz-frLsKFbz
> > jliE0&e=
> >
> > At boot time the i40e driver complains about it with the following
> > message but continues.
> >
> > MFS for port 1 has been set below the default: 600
> >
> > If the MTU size is increased, the driver accepts it but large packets
> > will not be processed by the firmware generating tx_errors. The issue
> > is pretty silent for users. i.e doing TCP in such context will
> > generates lots of retransmissions until the proper window size (below
> 1500) will be used.
> >
> > To fix this case, it would have been ideal to increase the MFS, via
> > i40e_aqc_opc_set_mac_config, incoming patch will take care of it.
> >
> > At least, commit prevents setting up an MTU greater than the current
> MFS.
> > It will avoid being in the position of having an MTU set to 9000 on
> > the netdev with a firmware refusing packets larger than 1536.
> >
> > A typical trace looks like:
> > [ 377.548696] i40e 0000:5d:00.0 eno5: Error changing mtu to 9000, Max
> is 1500. MFS is too small.
> >
>
> Hi Erwan, all,
>
> As a fix, I think this patch warrants a fixes tag.
> Perhaps this one is appropriate?
>
> Fixes: 41c445ff0f48 ("i40e: main driver core")
>
> > Signed-off-by: Erwan Velu <e.velu@...teo.com>
> > ---
> > drivers/net/ethernet/intel/i40e/i40e_main.c | 10 +++++++++-
> > 1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > index f86578857e8a..85ecf2f3de18 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_main.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
> > @@ -2946,7 +2946,7 @@ static int i40e_change_mtu(struct net_device
> *netdev, int new_mtu)
> > struct i40e_netdev_priv *np = netdev_priv(netdev);
> > struct i40e_vsi *vsi = np->vsi;
> > struct i40e_pf *pf = vsi->back;
> > - int frame_size;
> > + int frame_size, mfs, max_mtu;
> >
> > frame_size = i40e_max_vsi_frame_size(vsi, vsi->xdp_prog);
> > if (new_mtu > frame_size - I40E_PACKET_HDR_PAD) {
>
> I am fine with this patch, so please take what follows as a suggestion for
> improvement, possibly as a follow-up. Not as a hard requirement from my
> side.
>
> The part of this function between the two hunks of this patch is:
>
> netdev_err(netdev, "Error changing mtu to %d, Max is
> %d\n",
> new_mtu, frame_size - I40E_PACKET_HDR_PAD);
>
> My reading is that with this patch two different limits are checked wrt
> maximum MTU size:
>
> 1. A VSI level limit, which relates to RX buffer size 2. A PHY level limit that
> relates to the MFS
>
> That seems fine to me. But the log message for 1 (above) does not seem
> particularly informative wrt which limit has been exceeded.
>
> > @@ -2955,6 +2955,14 @@ static int i40e_change_mtu(struct net_device
> *netdev, int new_mtu)
> > return -EINVAL;
> > }
> >
> > + mfs = pf->hw.phy.link_info.max_frame_size;
> > + max_mtu = mfs - I40E_PACKET_HDR_PAD;
> > + if (new_mtu > max_mtu) {
> > + netdev_err(netdev, "Error changing mtu to %d, Max is %d.
> MFS is too small.\n",
> > + new_mtu, max_mtu);
> > + return -EINVAL;
> > + }
> > +
Aren't these driver specific checks and messages deprecated in favor generic one ?
Is it not possible to do below
@@ -1607,6 +1607,7 @@ int i40e_aq_get_link_info(struct i40e_hw *hw,
hw_link_info->ext_info = resp->ext_info;
hw_link_info->loopback = resp->loopback & I40E_AQ_LOOPBACK_MASK;
hw_link_info->max_frame_size = le16_to_cpu(resp->max_frame_size);
+ netdev->max_mtu = hw_link_info->max_frame_size - I40E_PACKET_HDR_PAD;
hw_link_info->pacing = resp->config & I40E_AQ_CONFIG_PACING_MASK;
So that stack will take care checking max MTU.
Thanks,
Sunil.
Powered by blists - more mailing lists