From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/HVM: avoid pointer wraparound in bufioreq handling Date: Tue, 16 Jun 2015 10:32:34 +0100 Message-ID: <557FED32.8050509@citrix.com> References: <557EFDA10200007800084F23@mail.emea.novell.com> <557EF572.2010302@citrix.com> <557FE116020000780008537A@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z4nEb-0000ox-DS for xen-devel@lists.xenproject.org; Tue, 16 Jun 2015 09:32:45 +0000 In-Reply-To: <557FE116020000780008537A@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 16/06/15 07:40, Jan Beulich wrote: >>>> On 15.06.15 at 17:55, wrote: >> On 15/06/15 15:30, Jan Beulich wrote: >>> + /* Canonicalize read/write pointers to prevent their overflow. */ >>> + while ( s->bufioreq_atomic && >>> + 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 has the possibility for a misbehaving emulator to livelock Xen by >> playing with the pointers. >> >> I think you need to break and kill the ioreq server if the read pointer >> is ever observed going backwards, or overtaking the write pointer. It >> is however legitimate to observe the read pointer stepping forwards one >> entry at a time, as processing is occurring. > Watching for the pointer to step backwards isn't nice; what I > would do instead is to limit the loop count here to > IOREQ_BUFFER_SLOT_NUM (on the basis that we're not > creating new entries, and hence the reader can't legitimately > update the pointer more than that number of times); for > simplicity's sake I wouldn't try to limit the loop further (e.g. to > write_pointer - read_pointer iterations). That seems like a reasonable compromise. 511 cmpxchg()s isn't over the top in terms of time. ~Andrew