Re: [PATCH net-next v4] xen-netback: Adding debugfs "io_ring_qX" files

[Wei Liu <wei.liu2@citrix.com>]

On Mon, Aug 11, 2014 at 01:17:48PM +0100, Zoltan Kiss wrote:
> On 11/08/14 11:38, Wei Liu wrote:
> >On Sun, Aug 10, 2014 at 10:57:51PM +0800, SeeChen Ng wrote:
> >>Hi, I'm a noob to linux kernel, I tried to figure out the usage of the function
> >>"simple_write_to_buffer", and I got confused about the code here.
> >>
> >>>+static ssize_t
> >>>+xenvif_write_io_ring(struct file *filp, const char __user *buf, size_t count,
> >>>+                    loff_t *ppos)
> >>>+{
> >>>+       struct xenvif_queue *queue =
> >>>+               ((struct seq_file *)filp->private_data)->private;
> >>>+       int len;
> >>>+       char write[sizeof(XENVIF_KICK_STR)];
> >>>+
> >>>+       /* don't allow partial writes and check the length */
> >>>+       if (*ppos != 0)
> >>>+               return 0;
> >>>+       if (count < sizeof(XENVIF_KICK_STR) - 1)
> >>>+               return -ENOSPC;
> >>The statement here is trying to verify the value of "count" is smaller
> >>than the size of array "write", make sure that the array got
> >>enough space for the write operation, right?
> >>
> >
> >Yes I think so.
> >
> >>So, I think the statement should be:
> >>
> >>          if (count >= sizeof(XENVIF_KICK_STR))
> >>                  return -ENOSPC;
> >>
> >
> >  * sizeof(XENVIF_KICK_STR) = 5
> >  * count is the number of bytes needs to be written, in the case of "kick"
> "count" is the number of bytes in the incoming buffer. We don't necessarily
> need all of them.
> 
> >    it's 5 because the tailing '\0' is also counted.
> >
> >so the correct fix should be
> >
> >   if (count > sizeof(XENVIF_KICK_STR))
> >           return -ENOSPC;
> That would fail if e.g. there is a newline character after the 4 letters of
> "kick". I've crafted this check in this way so we don't have to worry what
> is after that 4 character. This check makes sure there is at least 4
> character, and strncmp check exactly those ones. If there is anything after
> that, we don't care about it.
> 

I think current code is wrong in two ways.

a) echo "k" > io_ring_q0 -> returns -ENOSPC
   when there's still space in the array, -EINVAL is more appropriate

b) echo "kick1234" > io_ring_q0 -> succeeds but "kick1234" is not a
   defined command

My preferred option here is to check the lenght of the input for
excactly what you want. If you only support "kick", that means you
*only* allow the input to be "kick", nothing less, nothing more.

In any case -ENOSPC when there is still space is wrong. -EINVAL is more
appropriate.

So another fix will be

    if (count is less than expected)
      return -EINVAL;
    if (count is more than expected)
      return -ENOSPC;

Wei.