[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <BY3PR18MB461280AEEC17DB42F2926D74ABA89@BY3PR18MB4612.namprd18.prod.outlook.com>
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