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

From: Oleksandr Andrushchenko
Date: Wed Apr 25 2018 - 04:26:47 EST


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