netdev - Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date: Tue, 19 Mar 2024 12:20:24 +0000
From: Simon Horman <horms@...nel.org>
To: Erwan Velu <e.velu@...teo.com>
Cc: Erwan Velu <erwanaliasr1@...il.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: Re: [PATCH v4 iwl-net] i40e: Prevent setting MTU if greater than MFS

On Tue, Mar 19, 2024 at 12:38:03PM +0100, Erwan Velu wrote:
> 
> Le 18/03/2024 à 18:45, Simon Horman a écrit :
> > [...]
> > 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")
> 
> Simon
> 
> Isn't that a bit too generic ?

Yes, maybe it is.
What we would be after is the first commit where the
user can hit the problem the patch addresses.

> [..]
> 
> > 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.
> 
> I got some comments around this.
> 
> I wanted to keep my patch being focused on the mfs issue, but I can offer a
> patch to get a similar output for this. What WRT stands for ?
> 
> 
> I wanted also to make another patch for this :
> 
> dev_warn(&pdev->dev, "MFS for port %x has been set below the default:
> %x\n",pf->hw.port, val);
> 
> The MFS reported as hex without a "0x" prefix is very misleading, I can
> offer a patch for this too.

FWIIW, I think handling these questions in follow-up patches is fine.