Re: [Qemu-devel] [PATCH] qemu-char: fix tcp_get_fds

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Nov 03, 2014 at 04:09:36PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > tcp_get_fds API discards fds if there's more than 1 of these.
> 
> s/tcp_get_fds/tcp_get_msgfds/ (subject as well)

Right. Too late as I sent this upstream :(

> What exactly doesn't work without this patch?

It's only used by vhost test. It works by chance because
it's only using 512m ram.

I tweaked vhost user test
to use more memory (3900 instead of 512 M) and it started failing
because it needs 3 fds then.

Not yet upstreaming the test change itself, looking
for ways to avoid using huge pages for this.

> > It's tricky to fix this without API changes in the generic case.
> >
> > However, this API is only used by tests ATM, and tests know how
> > many fds they expect.
> >
> > So let's not waste cycles trying to fix this properly:
> > simply assume at most 16 fds (tests use at most 8 now).
> > assert if some test tries to get more.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  qemu-char.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/qemu-char.c b/qemu-char.c
> > index bd0709b..1c4004c 100644
> > --- a/qemu-char.c
> > +++ b/qemu-char.c
> > @@ -88,6 +88,7 @@
> >  #define READ_BUF_LEN 4096
> >  #define READ_RETRIES 10
> >  #define CHR_MAX_FILENAME_SIZE 256
> > +#define TCP_MAX_FDS 16
> >  
> >  /***********************************************************/
> >  /* Socket address helpers */
> > @@ -2668,6 +2669,8 @@ static int tcp_get_msgfds(CharDriverState *chr, int *fds, int num)
> >      TCPCharDriver *s = chr->opaque;
> >      int to_copy = (s->read_msgfds_num < num) ? s->read_msgfds_num : num;
> >  
> > +    assert(num <= TCP_MAX_FDS);
> > +
> >      if (to_copy) {
> >          int i;
> >  
> 
> This where we copy received fds out of ->read_msgfds.  If someone asks
> for more than TCP_MAX_FDS, the buffer in the next hunk is insufficient.
> > @@ -2762,7 +2765,7 @@ static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len)
> >      struct iovec iov[1];
> >      union {
> >          struct cmsghdr cmsg;
> > -        char control[CMSG_SPACE(sizeof(int))];
> > +        char control[CMSG_SPACE(sizeof(int) * TCP_MAX_FDS)];
> >      } msg_control;
> >      int flags = 0;
> >      ssize_t ret;
> 
> This is where we receive the fds into ->read_msgfds.  How many depends
> on sizeof(msg_control).  One before your patch, TCP_MAX_FDS after.
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>