-----Original Message-----
From: Thomas Bogendoerfer <redacted>
Sent: Tuesday, August 27, 2024 12:12 PM
To: Kitszel, Przemyslaw <przemyslaw.kitszel@intel.com>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; David S. Miller
[off-list ref]; Eric Dumazet [off-list ref]; Jakub
Kicinski [off-list ref]; Paolo Abeni [off-list ref]; intel-
wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
kernel@vger.kernel.org; Ertman, David M [off-list ref]; Jiri
Pirko [off-list ref]
Subject: Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support
SRIOV_LAG

On Tue, 27 Aug 2024 09:16:51 +0200
Przemek Kitszel [off-list ref] wrote:

quoted

On 8/26/24 12:17, Thomas Bogendoerfer wrote:

quoted

On Mon, 26 Aug 2024 11:41:19 +0200
Jiri Pirko [off-list ref] wrote:

quoted

Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:

quoted

For PFs, which don't support SRIOV_LAG, there is no pf->lag struct
allocated. So before accessing pf->lag a NULL pointer check is needed.

Signed-off-by: Thomas Bogendoerfer <redacted>

You need to add a "fixes" tag blaming the commit that introduced the
bug.

Would be also good to CC the author.

sure, I'm using get_maintainer for building address line and looks
like it only adds the author, if there is a Fixes tag, which IMHO
makes more sense than mailing all possible authors of file (in this
case it would work, but there are other files).

quoted

quoted

Fixes: 1e0f9881ef79 ("ice: Flesh out implementation of support for
SRIOV on bonded interface")

the bug was introduced later, the tag should be:
Fixes: ec5a6c5f79ed ("ice: process events created by lag netdev event
handler")

I'd like to disagree, ec5a6c5f79ed adds an empty
ice_lag_move_new_vf_nodes(),
which will do no harm if pf->lag is NULL. Commit 1e0f9881ef79 introduces
the access to pf->lag without checking for NULL.

quoted

The mentioned commit extracted code into

ice_lag_move_new_vf_nodes(),

quoted

and there is just one call to this function by now, just after
releasing lag_mutex, so would be good to change the semantics of
ice_lag_move_new_vf_nodes() to "only for lag-enabled flows, with
lag_mutex held", and fix the call to it to reflect that.

I could do that for sure, but IMHO this is about fixing a bug,
which crashes the kernel. Making the code better should be done
after fixing.

Thomas,

Nice catch!

I looked into this a bit and it seems that when I sent in patch:
commit 9f74a3dfcf83 ("ice: Fix VF Reset paths when interface in a failed over aggregate)

I left in a spurious call to the previous function for moving nodes. Since it is
just in the error path it went unnoticed this long.

Since this is the only call to ice_lag_move_new_vf_nodes(), it seems that
proper way of fixing this would be to eliminate the spurious call and the function
definition entirely.

If you do no want to do this, I can volunteer to write the patch.

Thanks,
Dave Ertman

Thomas.

--
SUSE Software Solutions Germany GmbH
HRB 36809 (AG Nürnberg)
Geschäftsführer: Ivo Totev, Andrew McDonald, Werner Knoblich