Re: [PATCH] ibmvnic: fix krealloc() memory leak

[Alexander A. Klimov]

On 5/26/26 22:50, Nicolai Buchwitz wrote:
> Hi Alex
>
> You patch is missing the prefix with the target tree. Please have
> a look at [1] for more details on the workflow.

Damn. :-)

So far I've always got away with whatever I copied from the majority of
git log --oneline --follow FILE

> On 26.5.2026 20:41, Alexander A. Klimov wrote:
> > Don't just overwrite the original pointer passed to krealloc()
> > with its return value without checking latter:
> >
> >     MEM = krealloc(MEM, SZ, GFP);
> >
> > If krealloc() returns NULL, that erases the pointer
> > to the still allocated memory, hence leaks this memory.
> > Instead, use a temporary variable, check it's not NULL
> > and only then assign it to the original pointer:
> >
> >     TMP = krealloc(MEM, SZ, GFP);
> >     if (!TMP) return;
> >     MEM = TMP;
> >
> > Fixes: 4e6759be28e4 ("ibmvnic: Feature implementation of Vital Product Data (VPD) for the ibmvnic driver")
> > Signed-off-by: Alexander A. Klimov <grandmaster@xxxxxxxxxxxx>
> > ---
> >  drivers/net/ethernet/ibm/ibmvnic.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> > index 5a510eed335e..25d1d844ad19 100644
> > --- a/drivers/net/ethernet/ibm/ibmvnic.c
> > +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> > @@ -1761,8 +1761,9 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
> >      union ibmvnic_crq crq;
> >      int len = 0;
> >      int rc;
> > +    unsigned char *buff = adapter->vpd->buff;
>
> Should be reverse x-mas tree (longest to shortest).
>
> > -    if (adapter->vpd->buff)
> > +    if (buff)
> >          len = adapter->vpd->len;
> >
> >      mutex_lock(&adapter->fw_lock);
> > @@ -1788,17 +1789,17 @@ static int ibmvnic_get_vpd(struct ibmvnic_adapter *adapter)
> >      if (!adapter->vpd->len)
> >          return -ENODATA;
> >
> > -    if (!adapter->vpd->buff)
> > -        adapter->vpd->buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
> > +    if (!buff)
> > +        buff = kzalloc(adapter->vpd->len, GFP_KERNEL);
> >      else if (adapter->vpd->len != len)
> > -        adapter->vpd->buff =
> > -            krealloc(adapter->vpd->buff,
> > -                 adapter->vpd->len, GFP_KERNEL);
> > +        buff = krealloc(buff,
> > +                adapter->vpd->len, GFP_KERNEL);
>
> Dead branch? The only caller, init_resources(), kzalloc()s a fresh vpd
> right before, and resets run release_vpd_data() first, so vpd->buff is
> always NULL here and kzalloc() above always wins. The leak can't trigger,

Cool! No leak = no problem = nothing to do here