[PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Thomas Bogendoerfer]

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 <tbogendoerfer@suse.de>
---
 drivers/net/ethernet/intel/ice/ice_lag.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
index 1ccb572ce285..916a16a379a8 100644
--- a/drivers/net/ethernet/intel/ice/ice_lag.c
+++ b/drivers/net/ethernet/intel/ice/ice_lag.c
@@ -704,7 +704,7 @@ void ice_lag_move_new_vf_nodes(struct ice_vf *vf)
 	lag = pf->lag;
 
 	mutex_lock(&pf->lag_mutex);
-	if (!lag->bonded)
+	if (!lag || !lag->bonded)
 		goto new_vf_unlock;
 
 	pri_port = pf->hw.port_info->lport;
-- 
2.35.3

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Jiri Pirko]

Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:
>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 <tbogendoerfer@suse.de>

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


>---
> drivers/net/ethernet/intel/ice/ice_lag.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_lag.c b/drivers/net/ethernet/intel/ice/ice_lag.c
>index 1ccb572ce285..916a16a379a8 100644
>--- a/drivers/net/ethernet/intel/ice/ice_lag.c
>+++ b/drivers/net/ethernet/intel/ice/ice_lag.c
>@@ -704,7 +704,7 @@ void ice_lag_move_new_vf_nodes(struct ice_vf *vf)
> 	lag = pf->lag;
> 
> 	mutex_lock(&pf->lag_mutex);
>-	if (!lag->bonded)
>+	if (!lag || !lag->bonded)
> 		goto new_vf_unlock;
> 
> 	pri_port = pf->hw.port_info->lport;
>-- 
>2.35.3
>
>

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Thomas Bogendoerfer]

On Mon, 26 Aug 2024 11:41:19 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:
> >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 <tbogendoerfer@suse.de>  
> 
> You need to add a "fixes" tag blaming the commit that introduced the
> bug.

of course...

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

Should I resend the patch ?

Thomas.

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

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Jiri Pirko]

Mon, Aug 26, 2024 at 12:17:10PM CEST, tbogendoerfer@suse.de wrote:
>On Mon, 26 Aug 2024 11:41:19 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:
>> >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 <tbogendoerfer@suse.de>  
>> 
>> You need to add a "fixes" tag blaming the commit that introduced the
>> bug.
>
>of course...
>
>Fixes: 1e0f9881ef79 ("ice: Flesh out implementation of support for SRIOV on
>bonded interface")
>
>Should I resend the patch ?

Yes.

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

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Przemek Kitszel]

On 8/26/24 12:17, Thomas Bogendoerfer wrote:
> On Mon, 26 Aug 2024 11:41:19 +0200
> Jiri Pirko <jiri@resnulli.us> wrote:
> 
>> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:
>>> 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 <tbogendoerfer@suse.de>
>>
>> You need to add a "fixes" tag blaming the commit that introduced the
>> bug.

Would be also good to CC the author.

> 
> of course...
> 
> 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")

The mentioned commit extracted code into ice_lag_move_new_vf_nodes(),
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.

> 
> Should I resend the patch ?

sometimes that is not strictly needed, but after two possible tags
mentioned that's the best way :)

> 
> Thomas.
> 

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Thomas Bogendoerfer]

On Tue, 27 Aug 2024 09:16:51 +0200
Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote:

> On 8/26/24 12:17, Thomas Bogendoerfer wrote:
> > On Mon, 26 Aug 2024 11:41:19 +0200
> > Jiri Pirko <jiri@resnulli.us> wrote:
> >   
> >> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:  
> >>> 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 <tbogendoerfer@suse.de>  
> >>
> >> 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).

> > 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.
> 
> The mentioned commit extracted code into ice_lag_move_new_vf_nodes(),
> 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.

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

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Przemek Kitszel]

On 8/27/24 21:12, Thomas Bogendoerfer wrote:
> On Tue, 27 Aug 2024 09:16:51 +0200
> Przemek Kitszel <przemyslaw.kitszel@intel.com> wrote:
> 
>> On 8/26/24 12:17, Thomas Bogendoerfer wrote:
>>> On Mon, 26 Aug 2024 11:41:19 +0200
>>> Jiri Pirko <jiri@resnulli.us> wrote:
>>>    
>>>> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:
>>>>> 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 <tbogendoerfer@suse.de>
>>>>
>>>> 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).
> 
>>> 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.

Thanks for persistence, I do agree, will review v2.

>>
>> The mentioned commit extracted code into ice_lag_move_new_vf_nodes(),
>> 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.
> 

RE: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Ertman, David M]

> -----Original Message-----
> From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> 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
> <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; intel-
> wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Ertman, David M <david.m.ertman@intel.com>; Jiri
> Pirko <jiri@resnulli.us>
> 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 <przemyslaw.kitszel@intel.com> wrote:
> 
> > On 8/26/24 12:17, Thomas Bogendoerfer wrote:
> > > On Mon, 26 Aug 2024 11:41:19 +0200
> > > Jiri Pirko <jiri@resnulli.us> wrote:
> > >
> > >> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:
> > >>> 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 <tbogendoerfer@suse.de>
> > >>
> > >> 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).
> 
> > > 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.
> >
> > The mentioned commit extracted code into
> ice_lag_move_new_vf_nodes(),
> > 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

Re: [PATCH net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG

[Thomas Bogendoerfer]

On Fri, 30 Aug 2024 17:12:56 +0000
"Ertman, David M" <david.m.ertman@intel.com> wrote:

> > -----Original Message-----
> > From: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> > 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
> > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub
> > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; intel-
> > wired-lan@lists.osuosl.org; netdev@vger.kernel.org; linux-
> > kernel@vger.kernel.org; Ertman, David M <david.m.ertman@intel.com>; Jiri
> > Pirko <jiri@resnulli.us>
> > 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 <przemyslaw.kitszel@intel.com> wrote:
> >   
> > > On 8/26/24 12:17, Thomas Bogendoerfer wrote:  
> > > > On Mon, 26 Aug 2024 11:41:19 +0200
> > > > Jiri Pirko <jiri@resnulli.us> wrote:
> > > >  
> > > >> Mon, Aug 26, 2024 at 10:58:30AM CEST, tbogendoerfer@suse.de wrote:  
> > > >>> 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 <tbogendoerfer@suse.de>  
> > > >>
> > > >> 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).
> >   
> > > > 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.  
> > >
> > > The mentioned commit extracted code into  
> > ice_lag_move_new_vf_nodes(),  
> > > 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.

either way is fine. But shouldn't the fix alone just applied first ?
Who will pick it up ?

Thomas.

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