All of lore.kernel.org
 help / color / mirror / Atom feed
From: Greg Kurz <groug@kaod.org>
To: "Alastair D'Silva" <alastair@au1.ibm.com>
Cc: linuxppc-dev@lists.ozlabs.org,
	Christophe Lombard <clombard@linux.vnet.ibm.com>,
	Vaibhav Jain <vaibhav@linux.vnet.ibm.com>,
	stable@vger.kernel.org,
	Frederic Barrat <fbarrat@linux.vnet.ibm.com>,
	Andrew Donnellan <andrew.donnellan@au1.ibm.com>
Subject: Re: [PATCH] ocxl: Fix endiannes bug in ocxl_link_update_pe()
Date: Thu, 20 Dec 2018 15:53:27 +0100	[thread overview]
Message-ID: <20181220155327.1f963769@bahia.lan> (raw)
In-Reply-To: <facf5e15f5791dd8ddf92d25aee0d2c0a0cdda95.camel@au1.ibm.com>

On Mon, 17 Dec 2018 11:38:51 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:

> On Sun, 2018-12-16 at 22:28 +0100, Greg Kurz wrote:
> > All fields in the PE are big-endian. Use cpu_to_be32() like
> > everywhere
> > else something is written to the PE. Otherwise a wrong TID will be
> > used
> > by the NPU. If this TID happens to point to an existing thread
> > sharing
> > the same mm, it could be woken up by error. This is highly improbable
> > though. The likely outcome of this is the NPU not finding the target
> > thread and forcing the AFU into sending an interrupt, which userspace
> > is supposed to handle anyway.
> > 
> > Fixes: e948e06fc63a ("ocxl: Expose the thread_id needed for wait on
> > POWER9")
> > Cc: stable@vger.kernel.org      # v4.18
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > This bug remained unnoticed so far because the current OCXL test
> > suite
> > happens to call OCXL_IOCTL_ENABLE_P9_WAIT before attaching a context.
> > This causes ocxl_link_update_pe() to be called before
> > ocxl_link_add_pe()
> > which re-writes the TID in the PE with the appropriate endianness.
> > 
> > I have some patches that change the behavior of the OCXL test suite
> > so that
> > it can catch the issue:
> > 
> > https://github.com/gkurz/libocxl/commits/wake-host-thread-rework
> > ---
> >  drivers/misc/ocxl/link.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> > index 31695a078485..646d16450066 100644
> > --- a/drivers/misc/ocxl/link.c
> > +++ b/drivers/misc/ocxl/link.c
> > @@ -566,7 +566,7 @@ int ocxl_link_update_pe(void *link_handle, int
> > pasid, __u16 tid)
> >  
> >  	mutex_lock(&spa->spa_lock);
> >  
> > -	pe->tid = tid;
> > +	pe->tid = cpu_to_be32(tid);
> >  
> >  	/*
> >  	 * The barrier makes sure the PE is updated
> >   
> 
> Good catch, thanks.
> 
> Reviewed-by: Alastair D'Silva <alastair@d-silva.org>
> 

Friendly ping before Xmas break :)

WARNING: multiple messages have this Message-ID (diff)
From: Greg Kurz <groug@kaod.org>
To: "Alastair D'Silva" <alastair@au1.ibm.com>
Cc: Christophe Lombard <clombard@linux.vnet.ibm.com>,
	Vaibhav Jain <vaibhav@linux.vnet.ibm.com>,
	Frederic Barrat <fbarrat@linux.vnet.ibm.com>,
	stable@vger.kernel.org,
	Andrew Donnellan <andrew.donnellan@au1.ibm.com>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH] ocxl: Fix endiannes bug in ocxl_link_update_pe()
Date: Thu, 20 Dec 2018 15:53:27 +0100	[thread overview]
Message-ID: <20181220155327.1f963769@bahia.lan> (raw)
In-Reply-To: <facf5e15f5791dd8ddf92d25aee0d2c0a0cdda95.camel@au1.ibm.com>

On Mon, 17 Dec 2018 11:38:51 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:

> On Sun, 2018-12-16 at 22:28 +0100, Greg Kurz wrote:
> > All fields in the PE are big-endian. Use cpu_to_be32() like
> > everywhere
> > else something is written to the PE. Otherwise a wrong TID will be
> > used
> > by the NPU. If this TID happens to point to an existing thread
> > sharing
> > the same mm, it could be woken up by error. This is highly improbable
> > though. The likely outcome of this is the NPU not finding the target
> > thread and forcing the AFU into sending an interrupt, which userspace
> > is supposed to handle anyway.
> > 
> > Fixes: e948e06fc63a ("ocxl: Expose the thread_id needed for wait on
> > POWER9")
> > Cc: stable@vger.kernel.org      # v4.18
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> > 
> > This bug remained unnoticed so far because the current OCXL test
> > suite
> > happens to call OCXL_IOCTL_ENABLE_P9_WAIT before attaching a context.
> > This causes ocxl_link_update_pe() to be called before
> > ocxl_link_add_pe()
> > which re-writes the TID in the PE with the appropriate endianness.
> > 
> > I have some patches that change the behavior of the OCXL test suite
> > so that
> > it can catch the issue:
> > 
> > https://github.com/gkurz/libocxl/commits/wake-host-thread-rework
> > ---
> >  drivers/misc/ocxl/link.c |    2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/misc/ocxl/link.c b/drivers/misc/ocxl/link.c
> > index 31695a078485..646d16450066 100644
> > --- a/drivers/misc/ocxl/link.c
> > +++ b/drivers/misc/ocxl/link.c
> > @@ -566,7 +566,7 @@ int ocxl_link_update_pe(void *link_handle, int
> > pasid, __u16 tid)
> >  
> >  	mutex_lock(&spa->spa_lock);
> >  
> > -	pe->tid = tid;
> > +	pe->tid = cpu_to_be32(tid);
> >  
> >  	/*
> >  	 * The barrier makes sure the PE is updated
> >   
> 
> Good catch, thanks.
> 
> Reviewed-by: Alastair D'Silva <alastair@d-silva.org>
> 

Friendly ping before Xmas break :)

  reply	other threads:[~2018-12-20 15:12 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-16 21:28 [PATCH] ocxl: Fix endiannes bug in ocxl_link_update_pe() Greg Kurz
2018-12-16 21:28 ` Greg Kurz
2018-12-17  0:00 ` Andrew Donnellan
2018-12-17  0:00   ` Andrew Donnellan
2018-12-17  0:38 ` Alastair D'Silva
2018-12-17  0:38   ` Alastair D'Silva
2018-12-20 14:53   ` Greg Kurz [this message]
2018-12-20 14:53     ` Greg Kurz
2018-12-22  9:54 ` Michael Ellerman
2018-12-22  9:54   ` Michael Ellerman

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=20181220155327.1f963769@bahia.lan \
    --to=groug@kaod.org \
    --cc=alastair@au1.ibm.com \
    --cc=andrew.donnellan@au1.ibm.com \
    --cc=clombard@linux.vnet.ibm.com \
    --cc=fbarrat@linux.vnet.ibm.com \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=stable@vger.kernel.org \
    --cc=vaibhav@linux.vnet.ibm.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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.