LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: "Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>,
	xen-devel@lists.xenproject.org
Cc: stable@vger.kernel.org, Juergen Gross <jgross@suse.com>,
	Stefano Stabellini <sstabellini@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/6] xen: Add RING_COPY_RESPONSE()
Date: Mon, 30 Apr 2018 17:25:52 -0400	[thread overview]
Message-ID: <7bd2ff07-9d29-8dea-6f55-5ff24ddd433c@oracle.com> (raw)
In-Reply-To: <ba8ed01b01a81d79f6d1c59e08a318dc0309a95d.1525122026.git-series.marmarek@invisiblethingslab.com>

On 04/30/2018 05:01 PM, Marek Marczykowski-Górecki wrote:
> Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly
> (i.e., by not considering that the other end may alter the data in the
> shared ring while it is being inspected).  Safe usage of a response
> generally requires taking a local copy.
>
> Provide a RING_COPY_RESPONSE() macro to use instead of
> RING_GET_RESPONSE() and an open-coded memcpy().  This takes care of
> ensuring that the copy is done correctly regardless of any possible
> compiler optimizations.
>
> Use a volatile source to prevent the compiler from reordering or
> omitting the copy.
>
> This is complementary to XSA155.
>
> CC: stable@vger.kernel.org
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> ---
>  include/xen/interface/io/ring.h | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h
> index 3f40501..03702f6 100644
> --- a/include/xen/interface/io/ring.h
> +++ b/include/xen/interface/io/ring.h
> @@ -201,6 +201,20 @@ struct __name##_back_ring {						\
>  #define RING_GET_RESPONSE(_r, _idx)					\
>      (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
>  
> +/*
> + * Get a local copy of a response.
> + *
> + * Use this in preference to RING_GET_RESPONSE() so all processing is
> + * done on a local copy that cannot be modified by the other end.
> + *
> + * Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
> + * to be ineffective where _rsp is a struct which consists of only bitfields.
> + */
> +#define RING_COPY_RESPONSE(_r, _idx, _rsp) do {				\
> +	/* Use volatile to force the copy into _rsp. */			\
> +	*(_rsp) = *(volatile typeof(_rsp))RING_GET_RESPONSE(_r, _idx);	\
> +} while (0)
> +

To avoid repeating essentially the same comment, can you move
RING_COPY_RESPONSE definition next to RING_COPY_REQUEST and adjust the
existing comment? And probably move RING_GET_RESPONSE next to
RING_GET_REQUEST? In other words

#define RING_GET_REQUEST
#define RING_GET_RESPONSE

/* comment */
#define RING_COPY_REQUEST
#define RING_COPY_RESPONSE


Also, perhaps the two can be collapsed together, along the lines of

#define RING_COPY_(action, _r, _idx, _msg) do {                          \
        /* Use volatile to force the copy into _msg. */                 \
        *(_msg) = *(volatile typeof(_msg))RING_GET_##action(_r, _idx);   \
} while (0)

#define RING_COPY_REQUEST(_r, _idx, _req)  RING_COPY_(REQUEST, _r, _idx,
_req)
#define RING_COPY_RESPONSE(_r, _idx, _rsp)  RING_COPY_(RESPONSE, _r,
_idx, _rsp)


(I have not tried to compile this so it may well be wrong)

-boris



>  /* Loop termination condition: Would the specified index overflow the ring? */
>  #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)				\
>      (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))

  reply	other threads:[~2018-04-30 21:25 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-30 21:01 [PATCH 0/6] Fix XSA-155-like bugs in frontend drivers Marek Marczykowski-Górecki
2018-04-30 21:01 ` [PATCH 1/6] xen: Add RING_COPY_RESPONSE() Marek Marczykowski-Górecki
2018-04-30 21:25   ` Boris Ostrovsky [this message]
2018-04-30 21:27     ` Marek Marczykowski-Górecki
2018-04-30 21:41       ` Boris Ostrovsky
2018-04-30 21:01 ` [PATCH 2/6] xen-netfront: copy response out of shared buffer before accessing it Marek Marczykowski-Górecki
2018-05-02  5:20   ` [Xen-devel] " Oleksandr Andrushchenko
2018-04-30 21:01 ` [PATCH 3/6] xen-netfront: do not use data already exposed to backend Marek Marczykowski-Górecki
2018-04-30 21:01 ` [PATCH 4/6] xen-netfront: add range check for Tx response id Marek Marczykowski-Górecki
2018-05-01 10:05   ` [Xen-devel] " Wei Liu
2018-04-30 21:01 ` [PATCH 5/6] xen-blkfront: make local copy of response before using it Marek Marczykowski-Górecki
2018-04-30 21:01 ` [PATCH 6/6] xen-blkfront: prepare request locally, only then put it on the shared ring Marek Marczykowski-Górecki
2018-05-01  8:22   ` Roger Pau Monné
2018-05-01  9:15     ` [Xen-devel] " Roger Pau Monné

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=7bd2ff07-9d29-8dea-6f55-5ff24ddd433c@oracle.com \
    --to=boris.ostrovsky@oracle.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jgross@suse.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marmarek@invisiblethingslab.com \
    --cc=sstabellini@kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=xen-devel@lists.xenproject.org \
    --subject='Re: [PATCH 1/6] xen: Add RING_COPY_RESPONSE()' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).