Messages in this thread | | | Subject | Re: [PATCH 1/6] xen: Add RING_COPY_RESPONSE() | From | Boris Ostrovsky <> | Date | Mon, 30 Apr 2018 17:25:52 -0400 |
| |
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))
| |