[PATCH net] ixgbe: fix crash with empty VF macvlan list

[PATCH net] ixgbe: fix crash with empty VF macvlan list

[Dan Carpenter]

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

Re: [PATCH net] ixgbe: fix crash with empty VF macvlan list

[Simon Horman]

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
> 
> 

Re: [PATCH net] ixgbe: fix crash with empty VF macvlan list

[Dan Carpenter]

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

Re: [PATCH net] ixgbe: fix crash with empty VF macvlan list

[Simon Horman]

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.