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/...
[2] https://elixir.bootlin.com/linux/v4.17-rc2/source/drivers/block/xen-blkback/...