| Message ID | 3cee09b8-4c49-4a39-b889-75c0798dfe1c@moroto.mountain (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ixgbe: fix crash with empty VF macvlan list | expand |
On Thu, Oct 05, 2023 at 04:57:02PM +0300, Dan Carpenter wrote: > The adapter->vf_mvs.l list needs to be initialized even if the list is > empty. Otherwise it will lead to crashes. > > Fixes: c6bda30a06d9 ("ixgbe: Reconfigure SR-IOV Init") Hi Dan, I see that the patch cited above added the line you are changing. But it also seems to me that patch was moving it from elsewhere. Perhaps I am mistaken, but I wonder if this is a better tag. Fixes: a1cbb15c1397 ("ixgbe: Add macvlan support for VF") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > index a703ba975205..9cfdfa8a4355 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > @@ -28,6 +28,9 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, > struct vf_macvlans *mv_list; > int num_vf_macvlans, i; > > + /* Initialize list of VF macvlans */ > + INIT_LIST_HEAD(&adapter->vf_mvs.l); > + > num_vf_macvlans = hw->mac.num_rar_entries - > (IXGBE_MAX_PF_MACVLANS + 1 + num_vfs); > if (!num_vf_macvlans) > @@ -36,8 +39,6 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, > mv_list = kcalloc(num_vf_macvlans, sizeof(struct vf_macvlans), > GFP_KERNEL); > if (mv_list) { I'm not sure it it is worth it, but perhaps more conventional error handling could be used here: if (!mv_list) return; for (i = 0; i < num_vf_macvlans; i++) { ... > - /* Initialize list of VF macvlans */ > - INIT_LIST_HEAD(&adapter->vf_mvs.l); > for (i = 0; i < num_vf_macvlans; i++) { > mv_list[i].vf = -1; > mv_list[i].free = true; > -- > 2.39.2 > >
On Fri, Oct 06, 2023 at 01:16:27PM +0200, Simon Horman wrote: > On Thu, Oct 05, 2023 at 04:57:02PM +0300, Dan Carpenter wrote: > > The adapter->vf_mvs.l list needs to be initialized even if the list is > > empty. Otherwise it will lead to crashes. > > > > Fixes: c6bda30a06d9 ("ixgbe: Reconfigure SR-IOV Init") > > Hi Dan, > > I see that the patch cited above added the line you are changing. > But it also seems to me that patch was moving it from elsewhere. > > Perhaps I am mistaken, but I wonder if this is a better tag. > > Fixes: a1cbb15c1397 ("ixgbe: Add macvlan support for VF") > Yeah. You're right. I'll resend. > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > index a703ba975205..9cfdfa8a4355 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > @@ -28,6 +28,9 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, > > struct vf_macvlans *mv_list; > > int num_vf_macvlans, i; > > > > + /* Initialize list of VF macvlans */ > > + INIT_LIST_HEAD(&adapter->vf_mvs.l); > > + > > num_vf_macvlans = hw->mac.num_rar_entries - > > (IXGBE_MAX_PF_MACVLANS + 1 + num_vfs); > > if (!num_vf_macvlans) > > @@ -36,8 +39,6 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, > > mv_list = kcalloc(num_vf_macvlans, sizeof(struct vf_macvlans), > > GFP_KERNEL); > > if (mv_list) { > > I'm not sure it it is worth it, but perhaps more conventional error > handling could be used here: > > if (!mv_list) > return; > > for (i = 0; i < num_vf_macvlans; i++) { > ... I mean error handling is always cleaner than success handling but it's probably not worth cleaning up in old code. I say it's not worth cleaning up old code and yet I secretly reversed two if statements like this yesterday. :P https://lore.kernel.org/all/d9da4c97-0da9-499f-9a21-1f8e3f148dc1@moroto.mountain/ It really is nicer, yes. But it just makes the patch too noisy. regards, dan carpenter
On Fri, Oct 06, 2023 at 03:49:39PM +0300, Dan Carpenter wrote: > On Fri, Oct 06, 2023 at 01:16:27PM +0200, Simon Horman wrote: > > On Thu, Oct 05, 2023 at 04:57:02PM +0300, Dan Carpenter wrote: > > > The adapter->vf_mvs.l list needs to be initialized even if the list is > > > empty. Otherwise it will lead to crashes. > > > > > > Fixes: c6bda30a06d9 ("ixgbe: Reconfigure SR-IOV Init") > > > > Hi Dan, > > > > I see that the patch cited above added the line you are changing. > > But it also seems to me that patch was moving it from elsewhere. > > > > Perhaps I am mistaken, but I wonder if this is a better tag. > > > > Fixes: a1cbb15c1397 ("ixgbe: Add macvlan support for VF") > > > > Yeah. You're right. I'll resend. Thanks! > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 5 +++-- > > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > index a703ba975205..9cfdfa8a4355 100644 > > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c > > > @@ -28,6 +28,9 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, > > > struct vf_macvlans *mv_list; > > > int num_vf_macvlans, i; > > > > > > + /* Initialize list of VF macvlans */ > > > + INIT_LIST_HEAD(&adapter->vf_mvs.l); > > > + > > > num_vf_macvlans = hw->mac.num_rar_entries - > > > (IXGBE_MAX_PF_MACVLANS + 1 + num_vfs); > > > if (!num_vf_macvlans) > > > @@ -36,8 +39,6 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, > > > mv_list = kcalloc(num_vf_macvlans, sizeof(struct vf_macvlans), > > > GFP_KERNEL); > > > if (mv_list) { > > > > I'm not sure it it is worth it, but perhaps more conventional error > > handling could be used here: > > > > if (!mv_list) > > return; > > > > for (i = 0; i < num_vf_macvlans; i++) { > > ... > > I mean error handling is always cleaner than success handling but it's > probably not worth cleaning up in old code. I say it's not worth > cleaning up old code and yet I secretly reversed two if statements like > this yesterday. :P > https://lore.kernel.org/all/d9da4c97-0da9-499f-9a21-1f8e3f148dc1@moroto.mountain/ > It really is nicer, yes. But it just makes the patch too noisy. Yeah, I'm also worried about the noise in this case.
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c index a703ba975205..9cfdfa8a4355 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c @@ -28,6 +28,9 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, struct vf_macvlans *mv_list; int num_vf_macvlans, i; + /* Initialize list of VF macvlans */ + INIT_LIST_HEAD(&adapter->vf_mvs.l); + num_vf_macvlans = hw->mac.num_rar_entries - (IXGBE_MAX_PF_MACVLANS + 1 + num_vfs); if (!num_vf_macvlans) @@ -36,8 +39,6 @@ static inline void ixgbe_alloc_vf_macvlans(struct ixgbe_adapter *adapter, mv_list = kcalloc(num_vf_macvlans, sizeof(struct vf_macvlans), GFP_KERNEL); if (mv_list) { - /* Initialize list of VF macvlans */ - INIT_LIST_HEAD(&adapter->vf_mvs.l); for (i = 0; i < num_vf_macvlans; i++) { mv_list[i].vf = -1; mv_list[i].free = true;
The adapter->vf_mvs.l list needs to be initialized even if the list is empty. Otherwise it will lead to crashes. Fixes: c6bda30a06d9 ("ixgbe: Reconfigure SR-IOV Init") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)