On Mon, Nov 03, 2014 at 04:09:36PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <m...@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 <m...@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 <arm...@redhat.com>
