Re: [PATCH v2 3/5] ALSA: xen-front: Implement Xen event channel handling

[Oleksandr Andrushchenko]

On 04/24/2018 07:23 PM, Oleksandr Andrushchenko wrote:
> On 04/24/2018 06:02 PM, Takashi Iwai wrote:
> > On Tue, 24 Apr 2018 16:58:43 +0200,
> > Oleksandr Andrushchenko wrote:
> > > On 04/24/2018 05:35 PM, Takashi Iwai wrote:
> > > > On Tue, 24 Apr 2018 16:29:15 +0200,
> > > > Oleksandr Andrushchenko wrote:
> > > > > On 04/24/2018 05:20 PM, Takashi Iwai wrote:
> > > > > > On Mon, 16 Apr 2018 08:24:51 +0200,
> > > > > > Oleksandr Andrushchenko wrote:
> > > > > > > +static irqreturn_t evtchnl_interrupt_req(int irq, void *dev_id)
> > > > > > > +{
> > > > > > > +ÂÂÂ struct xen_snd_front_evtchnl *channel = dev_id;
> > > > > > > +ÂÂÂ struct xen_snd_front_info *front_info = channel->front_info;
> > > > > > > +ÂÂÂ struct xensnd_resp *resp;
> > > > > > > +ÂÂÂ RING_IDX i, rp;
> > > > > > > +ÂÂÂ unsigned long flags;
> > > > > > > +
> > > > > > > +ÂÂÂ if (unlikely(channel->state != EVTCHNL_STATE_CONNECTED))
> > > > > > > +ÂÂÂÂÂÂÂ return IRQ_HANDLED;
> > > > > > > +
> > > > > > > +ÂÂÂ spin_lock_irqsave(&front_info->io_lock, flags);
> > > > > > > +
> > > > > > > +again:
> > > > > > > +ÂÂÂ rp = channel->u.req.ring.sring->rsp_prod;
> > > > > > > +ÂÂÂ /* ensure we see queued responses up to rp */
> > > > > > > +ÂÂÂ rmb();
> > > > > > > +
> > > > > > > +ÂÂÂ for (i = channel->u.req.ring.rsp_cons; i != rp; i++) {
> > > > > > I'm not familiar with Xen stuff in general, but through a quick
> > > > > > glance, this kind of code worries me a bit.
> > > > > >
> > > > > > If channel->u.req.ring.rsp_cons has a bogus number, this may lead 
> > > > > > to a
> > > > > > very long loop, no? Better to have a sanity check of the ring 
> > > > > > buffer
> > > > > > size.
> > > > > In this loop I have:
> > > > > resp = RING_GET_RESPONSE(&channel->u.req.ring, i);
> > > > > and the RING_GET_RESPONSE macro is designed in the way that
> > > > > it wraps around when *i* in the question gets bigger than
> > > > > the ring size:
> > > > >
> > > > > #define RING_GET_REQUEST(_r, _idx)ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ ÂÂÂ \
> > > > > Â ÂÂÂ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
> > > > >
> > > > > So, even if the counter has a bogus number it will not last long
> > > > Hm, this prevents from accessing outside the ring buffer, but does it
> > > > change the loop behavior?
> > > no, it doesn't
> > > > Suppose channel->u.req.ring_rsp_cons = 1, and rp = 0, the loop below
> > > > would still consume the whole 32bit counts, no?
> > > >
> > > > ÂÂÂÂfor (i = channel->u.req.ring.rsp_cons; i != rp; i++) {
> > > > ÂÂÂÂÂÂÂ resp = RING_GET_RESPONSE(&channel->u.req.ring, i);
> > > > ÂÂÂÂÂÂÂ ...
> > > > ÂÂÂÂ}
> > > You are right here and the comment is totally valid.
> > > I'll put an additional check like here [1] and here [2]
> > > Will this address your comment?
> > Yep, this kind of sanity checks should work.
> Great, will implement the checks this way then
Well, after thinking a bit more on that and chatting on #xendevel IRC
with Juergen (he is on CC list), it seems that the way the code is now
it is all fine without the checks: the assumption here is that
the backend is trusted to always write sane values to the ring counters,
thus no overflow checks on frontend side are required.
Even if I implement the checks then I have no means to recover, but just 
print
an error message and bail out not handling any responses.
This is probably why the checks [1] and [2] are only implemented for the
backend side and there are no such macros for the frontend side.

Takashi, please let me know if the above sounds reasonable and
addresses your comments.
> > thanks,
> >
> > Takashi
> Thank you,
> Oleksandr
> > > > Takashi
> > > Thank you,
> > > Oleksandr
> > >
> > > [1]
> > > https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1127 
> > >
> > > [2]
> > > https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/blkback.c#L1135