drivers/net/ethernet/intel/idpf/idpf_idc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
If auxiliary_device_add() fails, idpf_plug_vport_aux_dev() calls
auxiliary_device_uninit(adev), whose release callback
idpf_vport_adev_release() frees the containing
struct iidc_rdma_vport_auxiliary_dev.
The current error path then accesses adev->id and later frees iadev
again, which may lead to a use-after-free and double free.
The issue was identified by a static analysis tool I developed and
confirmed by manual review.
Fix it by storing the allocated auxiliary device id in a local
variable and avoiding direct freeing of iadev after
auxiliary_device_uninit().
Fixes: be91128c579c ("idpf: implement RDMA vport auxiliary dev create, init, and destroy")
Cc: stable@vger.kernel.org
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
v2:
- note that the issue was identified by my static analysis tool
- and confirmed by manual review
drivers/net/ethernet/intel/idpf/idpf_idc.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c
index 6dad0593f7f2..2a18907643fc 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_idc.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c
@@ -59,6 +59,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
char name[IDPF_IDC_MAX_ADEV_NAME_LEN];
struct auxiliary_device *adev;
int ret;
+ int adev_id;
iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
if (!iadev)
@@ -74,11 +75,14 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
goto err_ida_alloc;
}
adev->id = ret;
+ adev->id = adev_id;
adev->dev.release = idpf_vport_adev_release;
adev->dev.parent = &cdev_info->pdev->dev;
sprintf(name, "%04x.rdma.vdev", cdev_info->pdev->vendor);
adev->name = name;
+ /* iadev is owned by the auxiliary device */
+ iadev = NULL;
ret = auxiliary_device_init(adev);
if (ret)
goto err_aux_dev_init;
@@ -92,7 +96,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
err_aux_dev_add:
auxiliary_device_uninit(adev);
err_aux_dev_init:
- ida_free(&idpf_idc_ida, adev->id);
+ ida_free(&idpf_idc_ida, adev_id);
err_ida_alloc:
vdev_info->adev = NULL;
kfree(iadev);
--
2.43.0On 4/13/2026 4:20 AM, Guangshuo Li wrote:
> If auxiliary_device_add() fails, idpf_plug_vport_aux_dev() calls
> auxiliary_device_uninit(adev), whose release callback
> idpf_vport_adev_release() frees the containing
> struct iidc_rdma_vport_auxiliary_dev.
>
> The current error path then accesses adev->id and later frees iadev
> again, which may lead to a use-after-free and double free.
>
> The issue was identified by a static analysis tool I developed and
> confirmed by manual review.
>
> Fix it by storing the allocated auxiliary device id in a local
> variable and avoiding direct freeing of iadev after
> auxiliary_device_uninit().
>
> Fixes: be91128c579c ("idpf: implement RDMA vport auxiliary dev create, init, and destroy")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
> ---
This doesn't look right. The commit message analysis seems to match this
fix from Greg KH:
https://lore.kernel.org/intel-wired-lan/2026041432-tapestry-condition-22ff@gregkh/
But the changes do not make any sense to me. It looks like a poorly done
AI-generated "fix" which is not correct. Greg's version does look like
it properly resolves this.
> v2:
> - note that the issue was identified by my static analysis tool
> - and confirmed by manual review
>
What even is this change log?? I see that version was sent and everyone
else was sane enough to just silently reject or ignore the v1...
> drivers/net/ethernet/intel/idpf/idpf_idc.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c
> index 6dad0593f7f2..2a18907643fc 100644
> --- a/drivers/net/ethernet/intel/idpf/idpf_idc.c
> +++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c
> @@ -59,6 +59,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
> char name[IDPF_IDC_MAX_ADEV_NAME_LEN];
> struct auxiliary_device *adev;
> int ret;
> + int adev_id;
>
You create a local variable here...
> iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
> if (!iadev)
> @@ -74,11 +75,14 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
> goto err_ida_alloc;
> }
> adev->id = ret;
> + adev->id = adev_id;
adev_is is never initialized, so you assign a random garbage
uninitialized value. This is obviously wrong and will lead to worse
errors than the failed cleanup.
I'm rejecting this patch in favor of the clearly appropriate fix from Greg.
> adev->dev.release = idpf_vport_adev_release;
> adev->dev.parent = &cdev_info->pdev->dev;
> sprintf(name, "%04x.rdma.vdev", cdev_info->pdev->vendor);
> adev->name = name;
>
> + /* iadev is owned by the auxiliary device */
> + iadev = NULL;> ret = auxiliary_device_init(adev);
> if (ret)
> goto err_aux_dev_init;
> @@ -92,7 +96,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info,
> err_aux_dev_add:
> auxiliary_device_uninit(adev);
> err_aux_dev_init:
> - ida_free(&idpf_idc_ida, adev->id);
> + ida_free(&idpf_idc_ida, adev_id);
> err_ida_alloc:
> vdev_info->adev = NULL;
> kfree(iadev);
Hi Jacob, Thanks for reviewing. On Wed, 15 Apr 2026 at 05:03, Jacob Keller <jacob.e.keller@intel.com> wrote: > > > This doesn't look right. The commit message analysis seems to match this > fix from Greg KH: > > https://lore.kernel.org/intel-wired-lan/2026041432-tapestry-condition-22ff@gregkh/ > > But the changes do not make any sense to me. It looks like a poorly done > AI-generated "fix" which is not correct. Greg's version does look like > it properly resolves this. > > > v2: > > - note that the issue was identified by my static analysis tool > > - and confirmed by manual review > > > > What even is this change log?? I see that version was sent and everyone > else was sane enough to just silently reject or ignore the v1... > > > drivers/net/ethernet/intel/idpf/idpf_idc.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c > > index 6dad0593f7f2..2a18907643fc 100644 > > --- a/drivers/net/ethernet/intel/idpf/idpf_idc.c > > +++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c > > @@ -59,6 +59,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info, > > char name[IDPF_IDC_MAX_ADEV_NAME_LEN]; > > struct auxiliary_device *adev; > > int ret; > > + int adev_id; > > > > You create a local variable here... > > > iadev = kzalloc(sizeof(*iadev), GFP_KERNEL); > > if (!iadev) > > @@ -74,11 +75,14 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info, > > goto err_ida_alloc; > > } > > adev->id = ret; > > + adev->id = adev_id; > > adev_is is never initialized, so you assign a random garbage > uninitialized value. This is obviously wrong and will lead to worse > errors than the failed cleanup. > > I'm rejecting this patch in favor of the clearly appropriate fix from Greg. > > > adev->dev.release = idpf_vport_adev_release; > > adev->dev.parent = &cdev_info->pdev->dev; > > sprintf(name, "%04x.rdma.vdev", cdev_info->pdev->vendor); > > adev->name = name; > > > > + /* iadev is owned by the auxiliary device */ > > + iadev = NULL;> ret = auxiliary_device_init(adev); > > if (ret) > > goto err_aux_dev_init; > > @@ -92,7 +96,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info, > > err_aux_dev_add: > > auxiliary_device_uninit(adev); > > err_aux_dev_init: > > - ida_free(&idpf_idc_ida, adev->id); > > + ida_free(&idpf_idc_ida, adev_id); > > err_ida_alloc: > > vdev_info->adev = NULL; > > kfree(iadev); > You are right that the v2 patch as sent is incomplete. That was my mistake when preparing/sending v2: it accidentally dropped the adev_id = ret; assignment, which made that version incorrect. For reference, the original v1 patch is here: https://lkml.org/lkml/2026/3/21/421 In v1, adev_id was assigned from ret before use, so I believe that particular uninitialized-variable issue was introduced in the v2 posting. Sorry for the confusion caused by the broken v2 posting. Thanks, Guangshuo
On 4/14/2026 6:47 PM, Guangshuo Li wrote: > Hi Jacob, > > Thanks for reviewing. > > On Wed, 15 Apr 2026 at 05:03, Jacob Keller <jacob.e.keller@intel.com> wrote: >> >> >> This doesn't look right. The commit message analysis seems to match this >> fix from Greg KH: >> >> https://lore.kernel.org/intel-wired-lan/2026041432-tapestry-condition-22ff@gregkh/ >> >> But the changes do not make any sense to me. It looks like a poorly done >> AI-generated "fix" which is not correct. Greg's version does look like >> it properly resolves this. >> >>> v2: >>> - note that the issue was identified by my static analysis tool >>> - and confirmed by manual review >>> >> >> What even is this change log?? I see that version was sent and everyone >> else was sane enough to just silently reject or ignore the v1... >> >>> drivers/net/ethernet/intel/idpf/idpf_idc.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/ethernet/intel/idpf/idpf_idc.c b/drivers/net/ethernet/intel/idpf/idpf_idc.c >>> index 6dad0593f7f2..2a18907643fc 100644 >>> --- a/drivers/net/ethernet/intel/idpf/idpf_idc.c >>> +++ b/drivers/net/ethernet/intel/idpf/idpf_idc.c >>> @@ -59,6 +59,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info, >>> char name[IDPF_IDC_MAX_ADEV_NAME_LEN]; >>> struct auxiliary_device *adev; >>> int ret; >>> + int adev_id; >>> >> >> You create a local variable here... >> >>> iadev = kzalloc(sizeof(*iadev), GFP_KERNEL); >>> if (!iadev) >>> @@ -74,11 +75,14 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info, >>> goto err_ida_alloc; >>> } >>> adev->id = ret; >>> + adev->id = adev_id; >> >> adev_is is never initialized, so you assign a random garbage >> uninitialized value. This is obviously wrong and will lead to worse >> errors than the failed cleanup. >> >> I'm rejecting this patch in favor of the clearly appropriate fix from Greg. >> >>> adev->dev.release = idpf_vport_adev_release; >>> adev->dev.parent = &cdev_info->pdev->dev; >>> sprintf(name, "%04x.rdma.vdev", cdev_info->pdev->vendor); >>> adev->name = name; >>> >>> + /* iadev is owned by the auxiliary device */ >>> + iadev = NULL;> ret = auxiliary_device_init(adev); >>> if (ret) >>> goto err_aux_dev_init; >>> @@ -92,7 +96,7 @@ static int idpf_plug_vport_aux_dev(struct iidc_rdma_core_dev_info *cdev_info, >>> err_aux_dev_add: >>> auxiliary_device_uninit(adev); >>> err_aux_dev_init: >>> - ida_free(&idpf_idc_ida, adev->id); >>> + ida_free(&idpf_idc_ida, adev_id); >>> err_ida_alloc: >>> vdev_info->adev = NULL; >>> kfree(iadev); >> > > You are right that the v2 patch as sent is incomplete. That was my > mistake when preparing/sending v2: it accidentally dropped the adev_id > = ret; assignment, which made that version incorrect. > > For reference, the original v1 patch is here: > > https://lkml.org/lkml/2026/3/21/421 > > In v1, adev_id was assigned from ret before use, so I believe that > particular uninitialized-variable issue was introduced in the v2 > posting. > > Sorry for the confusion caused by the broken v2 posting. No problem. I had missed the other version, which explains my confusion. Still, to my eyes, the fix looks to be an equivalent fix as one submitted by GregKH: https://lore.kernel.org/intel-wired-lan/2026041116-retail-bagginess-250f@gregkh/ Do you agree this is effectively a different fix for the same problem? Or is there really two different double-free issues here that both need patching? I haven't been able to fully convince my self either way, but I am leaning on this being one problem, and I think Gregs solution feels simpler to understand. Thanks, Jake > > Thanks, > Guangshuo
Hi Jacob, Thanks for reviewing. On Wed, 15 Apr 2026 at 13:37, Jacob Keller <jacob.e.keller@intel.com> wrote: > > No problem. I had missed the other version, which explains my confusion. > Still, to my eyes, the fix looks to be an equivalent fix as one > submitted by GregKH: > > https://lore.kernel.org/intel-wired-lan/2026041116-retail-bagginess-250f@gregkh/ > > Do you agree this is effectively a different fix for the same problem? > Or is there really two different double-free issues here that both need > patching? I haven't been able to fully convince my self either way, but > I am leaning on this being one problem, and I think Gregs solution feels > simpler to understand. > > Thanks, > Jake > > > > > Thanks, > > Guangshuo > Yes, I agree Greg's patch addresses the same underlying issue. For the other path in `idpf_plug_core_aux_dev()`, I had also previously sent a fix, for reference: v1: https://lkml.org/lkml/2026/3/18/1822 v2: https://lkml.org/lkml/2026/3/19/1285 The v2 for the core path was posted after discussion on the list and incorporated the feedback I received there. So my understanding is that Greg's patch covers the same class of issue in both places, while I had sent them as separate fixes. Thanks, Guangshuo
© 2016 - 2026 Red Hat, Inc.