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

[Nicolai Buchwitz <nb@tipi-net.de>]

Hi Alex

You patch is missing the prefix with the target tree. Please have
a look at [1] for more details on the workflow.

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@al2klimov.de>
> ---
>  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,
which makes the Fixes tag misleading.

> 
> -	if (!adapter->vpd->buff) {
> +	if (!buff) {
>  		dev_err(dev, "Could allocate VPD buffer\n");
>  		return -ENOMEM;
>  	}
> +	adapter->vpd->buff = buff;

If you keep it anyway: on failure the old buffer stays in vpd->buff 
while
vpd->len is already the new size, a mismatch the original avoided by
NULLing. kfree() it (krealloc() does not free on failure) and NULL 
before
-ENOMEM.

> 
>  	adapter->vpd->dma_addr =
>  		dma_map_single(dev, adapter->vpd->buff, adapter->vpd->len,

[1] https://docs.kernel.org/process/maintainer-netdev.html

Thanks,
Nicolai