[PATCH net] qede: avoid uninitialized entries in coal_entry array

[PATCH net] qede: avoid uninitialized entries in coal_entry array

[Michal Schmidt]

Even after commit 908d4bb7c54c ("qede: fix interrupt coalescing
configuration"), some entries of the coal_entry array may theoretically
be used uninitialized:

 1. qede_alloc_fp_array() allocates QEDE_MAX_RSS_CNT entries for
    coal_entry. The initial allocation uses kcalloc, so everything is
    initialized.
 2. The user sets a small number of queues (ethtool -L).
    coal_entry is reallocated for the actual small number of queues.
 3. The user sets a bigger number of queues.
    coal_entry is reallocated bigger. The added entries are not
    necessarily initialized.

In practice, the reallocations will actually keep using the originally
allocated region of memory, but we should not rely on it.

The reallocation is unnecessary. coal_entry can always have
QEDE_MAX_RSS_CNT entries.

Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/net/ethernet/qlogic/qede/qede_main.c | 21 +++++++-------------
 1 file changed, 7 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c b/drivers/net/ethernet/qlogic/qede/qede_main.c
index 4a3c3b5fb4a1..261f982ca40d 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_main.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
@@ -963,7 +963,6 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
 {
 	u8 fp_combined, fp_rx = edev->fp_num_rx;
 	struct qede_fastpath *fp;
-	void *mem;
 	int i;
 
 	edev->fp_array = kcalloc(QEDE_QUEUE_CNT(edev),
@@ -974,20 +973,14 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
 	}
 
 	if (!edev->coal_entry) {
-		mem = kcalloc(QEDE_MAX_RSS_CNT(edev),
-			      sizeof(*edev->coal_entry), GFP_KERNEL);
-	} else {
-		mem = krealloc(edev->coal_entry,
-			       QEDE_QUEUE_CNT(edev) * sizeof(*edev->coal_entry),
-			       GFP_KERNEL);
-	}
-
-	if (!mem) {
-		DP_ERR(edev, "coalesce entry allocation failed\n");
-		kfree(edev->coal_entry);
-		goto err;
+		edev->coal_entry = kcalloc(QEDE_MAX_RSS_CNT(edev),
+					   sizeof(*edev->coal_entry),
+					   GFP_KERNEL);
+		if (!edev->coal_entry) {
+			DP_ERR(edev, "coalesce entry allocation failed\n");
+			goto err;
+		}
 	}
-	edev->coal_entry = mem;
 
 	fp_combined = QEDE_QUEUE_CNT(edev) - fp_rx - edev->fp_num_tx;
 
-- 
2.39.1

RE: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry array

[Manish Chopra]

> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Friday, February 24, 2023 6:12 AM
> To: netdev@vger.kernel.org
> Cc: Manish Chopra <manishc@marvell.com>; Ariel Elior
> <aelior@marvell.com>; Alok Prasad <palok@marvell.com>
> Subject: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry array
> 
> External Email
> 
> ----------------------------------------------------------------------
> Even after commit 908d4bb7c54c ("qede: fix interrupt coalescing
> configuration"), some entries of the coal_entry array may theoretically be
> used uninitialized:
> 
>  1. qede_alloc_fp_array() allocates QEDE_MAX_RSS_CNT entries for
>     coal_entry. The initial allocation uses kcalloc, so everything is
>     initialized.
>  2. The user sets a small number of queues (ethtool -L).
>     coal_entry is reallocated for the actual small number of queues.
>  3. The user sets a bigger number of queues.
>     coal_entry is reallocated bigger. The added entries are not
>     necessarily initialized.
> 
> In practice, the reallocations will actually keep using the originally allocated
> region of memory, but we should not rely on it.
> 
> The reallocation is unnecessary. coal_entry can always have
> QEDE_MAX_RSS_CNT entries.

Hi Michal,

Reallocation is necessary here, the motivation for reallocation is commit b0ec5489c480
("qede: preserve per queue stats across up/down of interface"). The coalescing configuration
set from ethtool also needs to be retained across the interface state change, with this change
you are not going to retain anything but always initialize with default. IMO, reallocation will
always try to use same memory which was originally allocated if the requirement fits into it and
which is the case here (driver will not attempt to allocate anything extra which were originally
allocated ever). So there should not be any uninitialized contents, either they will be zero or
something which were configured from ethtool by the user previously.

Nacked-by: Manish Chopra <manishc@marvell.com>

Thanks,
Manish

> 
> Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> ---
>  drivers/net/ethernet/qlogic/qede/qede_main.c | 21 +++++++-------------
>  1 file changed, 7 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index 4a3c3b5fb4a1..261f982ca40d 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -963,7 +963,6 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
> {
>  	u8 fp_combined, fp_rx = edev->fp_num_rx;
>  	struct qede_fastpath *fp;
> -	void *mem;
>  	int i;
> 
>  	edev->fp_array = kcalloc(QEDE_QUEUE_CNT(edev), @@ -974,20
> +973,14 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
>  	}
> 
>  	if (!edev->coal_entry) {
> -		mem = kcalloc(QEDE_MAX_RSS_CNT(edev),
> -			      sizeof(*edev->coal_entry), GFP_KERNEL);
> -	} else {
> -		mem = krealloc(edev->coal_entry,
> -			       QEDE_QUEUE_CNT(edev) * sizeof(*edev-
> >coal_entry),
> -			       GFP_KERNEL);
> -	}
> -
> -	if (!mem) {
> -		DP_ERR(edev, "coalesce entry allocation failed\n");
> -		kfree(edev->coal_entry);
> -		goto err;
> +		edev->coal_entry = kcalloc(QEDE_MAX_RSS_CNT(edev),
> +					   sizeof(*edev->coal_entry),
> +					   GFP_KERNEL);
> +		if (!edev->coal_entry) {
> +			DP_ERR(edev, "coalesce entry allocation failed\n");
> +			goto err;
> +		}
>  	}
> -	edev->coal_entry = mem;
> 
>  	fp_combined = QEDE_QUEUE_CNT(edev) - fp_rx - edev->fp_num_tx;
> 
> --
> 2.39.1

Re: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry array

[Michal Schmidt]

[Sorry, I accidentally sent my reply with HTML, got rejected by the
list, so resending in plain text.]

On Fri, Feb 24, 2023 at 8:25 AM Manish Chopra <manishc@marvell.com> wrote:
>
> > -----Original Message-----
> > From: Michal Schmidt <mschmidt@redhat.com>
> > ----------------------------------------------------------------------
> > Even after commit 908d4bb7c54c ("qede: fix interrupt coalescing
> > configuration"), some entries of the coal_entry array may theoretically be
> > used uninitialized:
> >
> >  1. qede_alloc_fp_array() allocates QEDE_MAX_RSS_CNT entries for
> >     coal_entry. The initial allocation uses kcalloc, so everything is
> >     initialized.
> >  2. The user sets a small number of queues (ethtool -L).
> >     coal_entry is reallocated for the actual small number of queues.
> >  3. The user sets a bigger number of queues.
> >     coal_entry is reallocated bigger. The added entries are not
> >     necessarily initialized.
> >
> > In practice, the reallocations will actually keep using the originally allocated
> > region of memory, but we should not rely on it.
> >
> > The reallocation is unnecessary. coal_entry can always have
> > QEDE_MAX_RSS_CNT entries.
>
> Hi Michal,
>
> Reallocation is necessary here, the motivation for reallocation is commit b0ec5489c480
> ("qede: preserve per queue stats across up/down of interface"). The coalescing configuration
> set from ethtool also needs to be retained across the interface state change, with this change
> you are not going to retain anything but always initialize with default.

It is retained. edev->coal_entry is allocated the first time
qede_alloc_fp_array() executes for the net_device. And then it's never
freed until the destruction of the net_device in__qede_remove().

> IMO, reallocation will
> always try to use same memory which was originally allocated if the requirement fits into it and
> which is the case here (driver will not attempt to allocate anything extra which were originally
> allocated ever). So there should not be any uninitialized contents, either they will be zero or
> something which were configured from ethtool by the user previously.

Yes, that's what I wrote in my description ("In practice, [...]"). But
that's relying on an implementation detail. It is not a part of the realloc
contract. If you need all the contents preserved, then do not lie to the
kernel that you need only a small part of it, i.e. do not realloc to a
smaller size.

Michal

> Nacked-by: Manish Chopra <manishc@marvell.com>
>
> Thanks,
> Manish
>
> >
> > Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
> > Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> > ---
> >  drivers/net/ethernet/qlogic/qede/qede_main.c | 21 +++++++-------------
> >  1 file changed, 7 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > index 4a3c3b5fb4a1..261f982ca40d 100644
> > --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > @@ -963,7 +963,6 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
> > {
> >       u8 fp_combined, fp_rx = edev->fp_num_rx;
> >       struct qede_fastpath *fp;
> > -     void *mem;
> >       int i;
> >
> >       edev->fp_array = kcalloc(QEDE_QUEUE_CNT(edev), @@ -974,20
> > +973,14 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
> >       }
> >
> >       if (!edev->coal_entry) {
> > -             mem = kcalloc(QEDE_MAX_RSS_CNT(edev),
> > -                           sizeof(*edev->coal_entry), GFP_KERNEL);
> > -     } else {
> > -             mem = krealloc(edev->coal_entry,
> > -                            QEDE_QUEUE_CNT(edev) * sizeof(*edev-
> > >coal_entry),
> > -                            GFP_KERNEL);
> > -     }
> > -
> > -     if (!mem) {
> > -             DP_ERR(edev, "coalesce entry allocation failed\n");
> > -             kfree(edev->coal_entry);
> > -             goto err;
> > +             edev->coal_entry = kcalloc(QEDE_MAX_RSS_CNT(edev),
> > +                                        sizeof(*edev->coal_entry),
> > +                                        GFP_KERNEL);
> > +             if (!edev->coal_entry) {
> > +                     DP_ERR(edev, "coalesce entry allocation failed\n");
> > +                     goto err;
> > +             }
> >       }
> > -     edev->coal_entry = mem;
> >
> >       fp_combined = QEDE_QUEUE_CNT(edev) - fp_rx - edev->fp_num_tx;
> >
> > --
> > 2.39.1
>

RE: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry array

[Manish Chopra]

> -----Original Message-----
> From: Michal Schmidt <mschmidt@redhat.com>
> Sent: Friday, February 24, 2023 1:35 PM
> To: Manish Chopra <manishc@marvell.com>
> Cc: netdev@vger.kernel.org; Ariel Elior <aelior@marvell.com>; Alok Prasad
> <palok@marvell.com>
> Subject: Re: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry
> array
> 
> [Sorry, I accidentally sent my reply with HTML, got rejected by the list, so
> resending in plain text.]
> 
> On Fri, Feb 24, 2023 at 8:25 AM Manish Chopra <manishc@marvell.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Michal Schmidt <mschmidt@redhat.com>
> > > --------------------------------------------------------------------
> > > -- Even after commit 908d4bb7c54c ("qede: fix interrupt coalescing
> > > configuration"), some entries of the coal_entry array may
> > > theoretically be used uninitialized:
> > >
> > >  1. qede_alloc_fp_array() allocates QEDE_MAX_RSS_CNT entries for
> > >     coal_entry. The initial allocation uses kcalloc, so everything is
> > >     initialized.
> > >  2. The user sets a small number of queues (ethtool -L).
> > >     coal_entry is reallocated for the actual small number of queues.
> > >  3. The user sets a bigger number of queues.
> > >     coal_entry is reallocated bigger. The added entries are not
> > >     necessarily initialized.
> > >
> > > In practice, the reallocations will actually keep using the
> > > originally allocated region of memory, but we should not rely on it.
> > >
> > > The reallocation is unnecessary. coal_entry can always have
> > > QEDE_MAX_RSS_CNT entries.
> >
> > Hi Michal,
> >
> > Reallocation is necessary here, the motivation for reallocation is
> > commit b0ec5489c480
> > ("qede: preserve per queue stats across up/down of interface"). The
> > coalescing configuration set from ethtool also needs to be retained
> > across the interface state change, with this change you are not going to
> retain anything but always initialize with default.
> 
> It is retained. edev->coal_entry is allocated the first time
> qede_alloc_fp_array() executes for the net_device. And then it's never freed
> until the destruction of the net_device in__qede_remove().

I see, sorry for overlooking it.

> 
> > IMO, reallocation will
> > always try to use same memory which was originally allocated if the
> > requirement fits into it and which is the case here (driver will not
> > attempt to allocate anything extra which were originally allocated
> > ever). So there should not be any uninitialized contents, either they will be
> zero or something which were configured from ethtool by the user previously.
> 
> Yes, that's what I wrote in my description ("In practice, [...]"). But that's relying
> on an implementation detail. It is not a part of the realloc contract. If you
> need all the contents preserved, then do not lie to the kernel that you need
> only a small part of it, i.e. do not realloc to a smaller size.
> 

In that case, it makes sense !!
Acked-by: Manish Chopra <manishc@marvell.com>


> 
> > Nacked-by: Manish Chopra <manishc@marvell.com>
> >
> > Thanks,
> > Manish
> >
> > >
> > > Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
> > > Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
> > > ---
> > >  drivers/net/ethernet/qlogic/qede/qede_main.c | 21
> > > +++++++-------------
> > >  1 file changed, 7 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > > b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > > index 4a3c3b5fb4a1..261f982ca40d 100644
> > > --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > > +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > > @@ -963,7 +963,6 @@ static int qede_alloc_fp_array(struct qede_dev
> > > *edev) {
> > >       u8 fp_combined, fp_rx = edev->fp_num_rx;
> > >       struct qede_fastpath *fp;
> > > -     void *mem;
> > >       int i;
> > >
> > >       edev->fp_array = kcalloc(QEDE_QUEUE_CNT(edev), @@ -974,20
> > > +973,14 @@ static int qede_alloc_fp_array(struct qede_dev *edev)
> > >       }
> > >
> > >       if (!edev->coal_entry) {
> > > -             mem = kcalloc(QEDE_MAX_RSS_CNT(edev),
> > > -                           sizeof(*edev->coal_entry), GFP_KERNEL);
> > > -     } else {
> > > -             mem = krealloc(edev->coal_entry,
> > > -                            QEDE_QUEUE_CNT(edev) * sizeof(*edev-
> > > >coal_entry),
> > > -                            GFP_KERNEL);
> > > -     }
> > > -
> > > -     if (!mem) {
> > > -             DP_ERR(edev, "coalesce entry allocation failed\n");
> > > -             kfree(edev->coal_entry);
> > > -             goto err;
> > > +             edev->coal_entry = kcalloc(QEDE_MAX_RSS_CNT(edev),
> > > +                                        sizeof(*edev->coal_entry),
> > > +                                        GFP_KERNEL);
> > > +             if (!edev->coal_entry) {
> > > +                     DP_ERR(edev, "coalesce entry allocation failed\n");
> > > +                     goto err;
> > > +             }
> > >       }
> > > -     edev->coal_entry = mem;
> > >
> > >       fp_combined = QEDE_QUEUE_CNT(edev) - fp_rx - edev->fp_num_tx;
> > >
> > > --
> > > 2.39.1
> >

Re: [PATCH net] qede: avoid uninitialized entries in coal_entry array

[Jakub Kicinski]

On Fri, 24 Feb 2023 01:41:45 +0100 Michal Schmidt wrote:
> Even after commit 908d4bb7c54c ("qede: fix interrupt coalescing
> configuration"), some entries of the coal_entry array may theoretically
> be used uninitialized:
> 
>  1. qede_alloc_fp_array() allocates QEDE_MAX_RSS_CNT entries for
>     coal_entry. The initial allocation uses kcalloc, so everything is
>     initialized.
>  2. The user sets a small number of queues (ethtool -L).
>     coal_entry is reallocated for the actual small number of queues.
>  3. The user sets a bigger number of queues.
>     coal_entry is reallocated bigger. The added entries are not
>     necessarily initialized.
> 
> In practice, the reallocations will actually keep using the originally
> allocated region of memory, but we should not rely on it.
> 
> The reallocation is unnecessary. coal_entry can always have
> QEDE_MAX_RSS_CNT entries.
> 
> Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

Applied, thanks!