| Message ID | 20240826085830.28136-1-tbogendoerfer@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ice: Fix NULL pointer access, if PF doesn't support SRIOV_LAG | expand |
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 > >
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.
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
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. >
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.
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. >
> -----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
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.
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;
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(-)