From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Wei Liu <wei.liu2@citrix.com>,
Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
Ian Campbell <Ian.Campbell@eu.citrix.com>,
xen-devel <xen-devel@lists.xenproject.org>,
Keir Fraser <keir@xen.org>
Subject: Re: [PATCH v2] x86/HVM: avoid pointer wraparound in bufioreq handling
Date: Wed, 22 Jul 2015 15:49:49 +0100 [thread overview]
Message-ID: <alpine.DEB.2.02.1507221439190.17378@kaball.uk.xensource.com> (raw)
In-Reply-To: <55AFB8DB0200007800094131@prv-mh.provo.novell.com>
On Wed, 22 Jul 2015, Jan Beulich wrote:
> >>> On 21.07.15 at 18:18, <stefano.stabellini@eu.citrix.com> wrote:
> > On Thu, 18 Jun 2015, Jan Beulich wrote:
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -921,7 +921,7 @@ static void hvm_ioreq_server_disable(str
> >>
> >> static int hvm_ioreq_server_init(struct hvm_ioreq_server *s, struct domain *d,
> >> domid_t domid, bool_t is_default,
> >> - bool_t handle_bufioreq, ioservid_t id)
> >> + int bufioreq_handling, ioservid_t id)
> >
> > uint8_t?
>
> Why? I'm generally against using fixed width types when you don't
> really need them. And using uint_least8_t or uint_fast8_t is neither
> an opton, nor would it make the code look reasonable. Plain int is
> just fine here.
You are not just changing integer size but also switching from unsigned
to signed implicitly. I think it is not a good coding practice.
> >> @@ -2568,17 +2575,29 @@ int hvm_buffered_io_send(ioreq_t *p)
> >> return 0;
> >> }
> >>
> >> - pg->buf_ioreq[pg->write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
> >> + pg->buf_ioreq[pg->ptrs.write_pointer % IOREQ_BUFFER_SLOT_NUM] = bp;
> >>
> >> if ( qw )
> >> {
> >> bp.data = p->data >> 32;
> >> - pg->buf_ioreq[(pg->write_pointer+1) % IOREQ_BUFFER_SLOT_NUM] = bp;
> >> + pg->buf_ioreq[(pg->ptrs.write_pointer+1) % IOREQ_BUFFER_SLOT_NUM] = bp;
> >> }
> >>
> >> /* Make the ioreq_t visible /before/ write_pointer. */
> >> wmb();
> >> - pg->write_pointer += qw ? 2 : 1;
> >> + pg->ptrs.write_pointer += qw ? 2 : 1;
> >> +
> >> + /* Canonicalize read/write pointers to prevent their overflow. */
> >> + while ( s->bufioreq_atomic && qw++ < IOREQ_BUFFER_SLOT_NUM &&
> >> + pg->ptrs.read_pointer >= IOREQ_BUFFER_SLOT_NUM )
> >> + {
> >> + union bufioreq_pointers old = pg->ptrs, new;
> >> + unsigned int n = old.read_pointer / IOREQ_BUFFER_SLOT_NUM;
> >> +
> >> + new.read_pointer = old.read_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> >> + new.write_pointer = old.write_pointer - n * IOREQ_BUFFER_SLOT_NUM;
> >> + cmpxchg(&pg->ptrs.full, old.full, new.full);
> >> + }
> >
> > This is not safe: if the reader increments read_pointer (atomically)
> > after you read old.read_pointer and before cmpxchg, the increment is
> > lost.
>
> But that's what I use cmpxchg() for - if the value changes between
> the read and the cmpxchg, the latter will indicate so (for the purpose
> here simply by not updating the memory location), and the loop will go
> through another iteration.
Right, sorry, I read cmpxchg as xchg. I really don't know why. I think
that the code above works then.
> > I think you need to remove the cmpxchg and subtract a multiple of
> > IOREQ_BUFFER_SLOT_NUM from read_pointer atomically here. Unfortunately
> > if we do that, we cannot update both write_pointer and read_pointer
> > together anymore. But if we decrement write_pointer atomically before
> > read_pointer, I think we should be fine if we appropriately fix the
> > reader side too:
> >
> > QEMU:
> > atomic_read(&read_pointer);
> > read write_pointer;
> > xen_rmb();
> > while (read_pointer < write_pointer) {
>
> That would cause this comparison to possibly evaluate to "false", and
> I don't think leaving this loop early can bring any good.
Actually it would be fine because right after the loop we have:
notify_via_xen_event_channel(d, s->bufioreq_evtchn);
> >
> > [work]
> >
> > atomic_add(&read_pointer, stuff);
> > read write_pointer;
> > xen_rmb();
> > }
> >
> >
> > Xen:
> > [work]
> > increment write_pointer;
> > xen_wmb();
> >
> > if (read_pointer >= IOREQ_BUFFER_SLOT_NUM) {
> > write_pointer -= IOREQ_BUFFER_SLOT_NUM;
> > xen_wmb();
> > atomic_sub(read_pointer, IOREQ_BUFFER_SLOT_NUM);
> > }
next prev parent reply other threads:[~2015-07-22 14:51 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-06-18 13:14 [PATCH v2] x86/HVM: avoid pointer wraparound in bufioreq handling Jan Beulich
2015-06-18 13:41 ` Andrew Cooper
2015-07-21 16:18 ` Stefano Stabellini
2015-07-22 13:38 ` Jan Beulich
2015-07-22 14:49 ` Stefano Stabellini [this message]
2015-07-22 15:21 ` Jan Beulich
2015-07-22 15:45 ` Stefano Stabellini
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=alpine.DEB.2.02.1507221439190.17378@kaball.uk.xensource.com \
--to=stefano.stabellini@eu.citrix.com \
--cc=Ian.Campbell@eu.citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=andrew.cooper3@citrix.com \
--cc=keir@xen.org \
--cc=tim@xen.org \
--cc=wei.liu2@citrix.com \
--cc=xen-devel@lists.xenproject.org \
/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.