Re: [PATCH] RDMA/restrack: Fix potential invalid address access

[Wenchao Hao]

On 2024/3/7 17:13, Leon Romanovsky wrote:
> On Fri, Mar 01, 2024 at 05:55:15PM +0800, Wenchao Hao wrote:
> > struct rdma_restrack_entry's kern_name was set to KBUILD_MODNAME
> > in ib_create_cq(), while if the module exited but forgot del this
> > rdma_restrack_entry, it would cause a invalid address access in
> > rdma_restrack_clean() when print the owner of this rdma_restrack_entry.
> >
> > Fix this issue by using kstrdup() to set rdma_restrack_entry's
> > kern_name.
>
> I don't like kstrdup() and would like to avoid it, this rdma_restrack_clean()
> is purely for debugging and for a long time all upstream ULPs are "clean"
> from these not-released bugs.
>
> So my suggestion is to delete that part of code and it will be good enough.

It's OK for me. When found this issue, my first plan is to remove the code, but
I do not know why these code is added, so decide to using kstrdup() to work around
it.

Then what to do next? Do I need to post another patch or you would fix it by yourself?

> diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> index 01a499a8b88d..27727138f188 100644
> --- a/drivers/infiniband/core/restrack.c
> +++ b/drivers/infiniband/core/restrack.c
> @@ -60,47 +60,14 @@ static const char *type2str(enum rdma_restrack_type type)
>   void rdma_restrack_clean(struct ib_device *dev)
>   {
>   	struct rdma_restrack_root *rt = dev->res;
> -	struct rdma_restrack_entry *e;
> -	char buf[TASK_COMM_LEN];
> -	bool found = false;
> -	const char *owner;
>   	int i;
>   
>   	for (i = 0 ; i < RDMA_RESTRACK_MAX; i++) {
>   		struct xarray *xa = &dev->res[i].xa;
>   
> -		if (!xa_empty(xa)) {
> -			unsigned long index;
> -
> -			if (!found) {
> -				pr_err("restrack: %s", CUT_HERE);
> -				dev_err(&dev->dev, "BUG: RESTRACK detected leak of resources\n");
> -			}
> -			xa_for_each(xa, index, e) {
> -				if (rdma_is_kernel_res(e)) {
> -					owner = e->kern_name;
> -				} else {
> -					/*
> -					 * There is no need to call get_task_struct here,
> -					 * because we can be here only if there are more
> -					 * get_task_struct() call than put_task_struct().
> -					 */
> -					get_task_comm(buf, e->task);
> -					owner = buf;
> -				}
> -
> -				pr_err("restrack: %s %s object allocated by %s is not freed\n",
> -				       rdma_is_kernel_res(e) ? "Kernel" :
> -							       "User",
> -				       type2str(e->type), owner);
> -			}
> -			found = true;
> -		}
> +		WARN_ON(!xa_empty(xa));
>   		xa_destroy(xa);
>   	}
> -	if (found)
> -		pr_err("restrack: %s", CUT_HERE);
> -
>   	kfree(rt);
>   }
>   
>
> > Signed-off-by: Wenchao Hao <haowenchao2@xxxxxxxxxx>
> > ---
> >   drivers/infiniband/core/restrack.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/restrack.c b/drivers/infiniband/core/restrack.c
> > index 01a499a8b88d..6605011c4edc 100644
> > --- a/drivers/infiniband/core/restrack.c
> > +++ b/drivers/infiniband/core/restrack.c
> > @@ -177,7 +177,8 @@ static void rdma_restrack_attach_task(struct rdma_restrack_entry *res,
> >   void rdma_restrack_set_name(struct rdma_restrack_entry *res, const char *caller)
> >   {
> >   	if (caller) {
> > -		res->kern_name = caller;
> > +		kfree(res->kern_name);
> > +		res->kern_name = kstrdup(caller, GFP_KERNEL);
> >   		return;
> >   	}
> >   
> > @@ -195,7 +196,7 @@ void rdma_restrack_parent_name(struct rdma_restrack_entry *dst,
> >   			       const struct rdma_restrack_entry *parent)
> >   {
> >   	if (rdma_is_kernel_res(parent))
> > -		dst->kern_name = parent->kern_name;
> > +		dst->kern_name = kstrdup(parent->kern_name, GFP_KERNEL);
> >   	else
> >   		rdma_restrack_attach_task(dst, parent->task);
> >   }
> > @@ -306,6 +307,7 @@ static void restrack_release(struct kref *kref)
> >   		put_task_struct(res->task);
> >   		res->task = NULL;
> >   	}
> > +	kfree(res->kern_name);
> >   	complete(&res->comp);
> >   }
> >   
> > --
> > 2.32.0