DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Xueming(Steven) Li" <xuemingl@nvidia.com>
To: "Harris, James R" <james.r.harris@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>
Subject: Re: [dpdk-dev] Bug with commit 64051bb1 (devargs: unify scratch buffer storage)
Date: Sat, 17 Apr 2021 14:59:31 +0000
Message-ID: <BY5PR12MB43245EF6C3314235B4BE23C8A14B9@BY5PR12MB4324.namprd12.prod.outlook.com> (raw)
In-Reply-To: <075F856D-D645-4639-A2E2-2F5BE29C8D76@intel.com>

Hi Jim,

> From: Harris, James R <james.r.harris@intel.com> 
> Sent: Saturday, April 17, 2021 6:05 AM
> To: dev@dpdk.org; Xueming(Steven) Li <xuemingl@nvidia.com>
> Subject: Bug with commit 64051bb1 (devargs: unify scratch buffer storage)
> 
> Hi,
> 
> SPDK has identified a regression with commit 64051bb1 (devargs: unify scratch buffer storage).  The issue seems to be with this part of the patch:
> 
> @@ -276,15 +287,8 @@ rte_devargs_insert(struct rte_devargs **da)
>                 if (strcmp(listed_da->bus->name, (*da)->bus->name) == 0 &&
>                                 strcmp(listed_da->name, (*da)->name) == 0) {
>                         /* device already in devargs list, must be updated */
> -                       listed_da->type = (*da)->type;
> -                       listed_da->policy = (*da)->policy;
> -                       free(listed_da->args);
> -                       listed_da->args = (*da)->args;
> -                       listed_da->bus = (*da)->bus;
> -                       listed_da->cls = (*da)->cls;
> -                       listed_da->bus_str = (*da)->bus_str;
> -                       listed_da->cls_str = (*da)->cls_str;
> -                       listed_da->data = (*da)->data;
> +                       rte_devargs_reset(listed_da);
> +                       *listed_da = **da;
>                         /* replace provided devargs with found one */
>                         free(*da);
>                         *da = listed_da;
> 
> 
> Previously the data members were copied one-by-one, preserving the pointers in the listed_da’s TAILQ_ENTRY.  But after this patch, rte_devargs_reset() zeroes the entire rte_devargs structure, including the pointers in the TAILQ_ENTRY.  If we do a subsequent rte_devargs_remove() on this same entry, we segfault since the TAILQ_ENTRY’s pointers are invalid.  There could be similar segfaults with any subsequent rte_devargs_insert() calls that require iterating the global list of devargs entries.
> 
> rte_devargs_insert() could manually copy the TAILQ_ENTRY pointers to *da before calling rte_devargs_reset() – that at least fixes the SPDK regression.  But it’s not clear to me how many of the other rte_devargs_reset() callsites added by this patch also need to be changed in some way.

Thanks for reporting this issue, your fix should work. Rte_devargs_reset() simply free and clear da->data field, not all of da.

I will send a patch to fix this, thanks again for pointing this out.

> 
> Thanks,
> 
> -Jim
> 



      reply	other threads:[~2021-04-17 14:59 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-16 22:04 Harris, James R
2021-04-17 14:59 ` Xueming(Steven) Li [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BY5PR12MB43245EF6C3314235B4BE23C8A14B9@BY5PR12MB4324.namprd12.prod.outlook.com \
    --to=xuemingl@nvidia.com \
    --cc=dev@dpdk.org \
    --cc=james.r.harris@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git