* [PATCH net] ixgbe: fix crash with empty VF macvlan list
@ 2023-10-05 13:57 Dan Carpenter
2023-10-06 11:16 ` Simon Horman
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-10-05 13:57 UTC (permalink / raw)
To: Greg Rose
Cc: Jesse Brandeburg, Tony Nguyen, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Jeff Kirsher, intel-wired-lan,
netdev, kernel-janitors
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(-)
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;
--
2.39.2
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] ixgbe: fix crash with empty VF macvlan list 2023-10-05 13:57 [PATCH net] ixgbe: fix crash with empty VF macvlan list Dan Carpenter @ 2023-10-06 11:16 ` Simon Horman 2023-10-06 12:49 ` Dan Carpenter 0 siblings, 1 reply; 4+ messages in thread From: Simon Horman @ 2023-10-06 11:16 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Rose, Jesse Brandeburg, Tony Nguyen, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jeff Kirsher, intel-wired-lan, netdev, kernel-janitors 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 > > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] ixgbe: fix crash with empty VF macvlan list 2023-10-06 11:16 ` Simon Horman @ 2023-10-06 12:49 ` Dan Carpenter 2023-10-06 13:24 ` Simon Horman 0 siblings, 1 reply; 4+ messages in thread From: Dan Carpenter @ 2023-10-06 12:49 UTC (permalink / raw) To: Simon Horman Cc: Greg Rose, Jesse Brandeburg, Tony Nguyen, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jeff Kirsher, intel-wired-lan, netdev, kernel-janitors 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net] ixgbe: fix crash with empty VF macvlan list 2023-10-06 12:49 ` Dan Carpenter @ 2023-10-06 13:24 ` Simon Horman 0 siblings, 0 replies; 4+ messages in thread From: Simon Horman @ 2023-10-06 13:24 UTC (permalink / raw) To: Dan Carpenter Cc: Greg Rose, Jesse Brandeburg, Tony Nguyen, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Jeff Kirsher, intel-wired-lan, netdev, kernel-janitors 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. ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-10-06 13:24 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-10-05 13:57 [PATCH net] ixgbe: fix crash with empty VF macvlan list Dan Carpenter 2023-10-06 11:16 ` Simon Horman 2023-10-06 12:49 ` Dan Carpenter 2023-10-06 13:24 ` Simon Horman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).