Date:   Fri, 24 Feb 2023 08:34:59 +0000
From:   Manish Chopra <manishc@...vell.com>
To:     Michal Schmidt <mschmidt@...hat.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ariel Elior <aelior@...vell.com>,
        Alok Prasad <palok@...vell.com>
Subject: RE: [EXT] [PATCH net] qede: avoid uninitialized entries in coal_entry
 array

> -----Original Message-----
> From: Michal Schmidt <mschmidt@...hat.com>
> Sent: Friday, February 24, 2023 1:35 PM
> To: Manish Chopra <manishc@...vell.com>
> Cc: netdev@...r.kernel.org; Ariel Elior <aelior@...vell.com>; Alok Prasad
> <palok@...vell.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@...vell.com>
> wrote:
> >
> > > -----Original Message-----
> > > From: Michal Schmidt <mschmidt@...hat.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@...vell.com>


> 
> > Nacked-by: Manish Chopra <manishc@...vell.com>
> >
> > Thanks,
> > Manish
> >
> > >
> > > Fixes: 908d4bb7c54c ("qede: fix interrupt coalescing configuration")
> > > Signed-off-by: Michal Schmidt <mschmidt@...hat.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
> >

Powered by blists - more mailing lists