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

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

[Michael S. Tsirkin]

tcp_get_fds API discards fds if there's more than 1 of these.

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;
 
@@ -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;
-- 
MST

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

[Markus Armbruster]

"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)

What exactly doesn't work without this patch?

> 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>

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

[Michael S. Tsirkin]

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>

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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 :(

Why not simply respin with a fixed commit message?  Or ask the
maintainer to fix it up on commit?

>> 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.

Would be a nice addition to the commit message.

> 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.

Okay :)

[...]

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> 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 :(

Oh, now I see: you already sent this in a pull request, after less than
20 hours on list.  Please don't do that except in dire emergency.  It
sabotages our review process.

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

[Peter Maydell]

On 3 November 2014 16:13, Markus Armbruster <armbru@redhat.com> wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> 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 :(
>
> Oh, now I see: you already sent this in a pull request, after less than
> 20 hours on list.  Please don't do that except in dire emergency.  It
> sabotages our review process.

...I haven't actually applied that yet, though. Since this
isn't a critical bugfix I think it would probably be reasonable
to respin the pull without this patch, so it has a bit more
chance for review. Since it's not a new feature there's no
need to rush to get it in before hardfreeze, right?

-- PMM

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

[Michael S. Tsirkin]

On Mon, Nov 03, 2014 at 04:22:57PM +0000, Peter Maydell wrote:
> On 3 November 2014 16:13, Markus Armbruster <armbru@redhat.com> wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> >> 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 :(
> >
> > Oh, now I see: you already sent this in a pull request, after less than
> > 20 hours on list.  Please don't do that except in dire emergency.  It
> > sabotages our review process.
> 
> ...I haven't actually applied that yet, though. Since this
> isn't a critical bugfix I think it would probably be reasonable
> to respin the pull without this patch, so it has a bit more
> chance for review. Since it's not a new feature there's no
> need to rush to get it in before hardfreeze, right?
> 
> -- PMM

Okay.

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

[Michael S. Tsirkin]

On Mon, Nov 03, 2014 at 04:22:57PM +0000, Peter Maydell wrote:
> On 3 November 2014 16:13, Markus Armbruster <armbru@redhat.com> wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >
> >> 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 :(
> >
> > Oh, now I see: you already sent this in a pull request, after less than
> > 20 hours on list.  Please don't do that except in dire emergency.  It
> > sabotages our review process.
> 
> ...I haven't actually applied that yet, though. Since this
> isn't a critical bugfix I think it would probably be reasonable
> to respin the pull without this patch, so it has a bit more
> chance for review. Since it's not a new feature there's no
> need to rush to get it in before hardfreeze, right?
> 
> -- PMM

Hi Peter,

OK I backed that patch out and re-pushed.
As other patches are unchanged, I don't want to re-do the pull
request, it's just spam on list.

Tag name is still for_upstream, new hash
d43f0d641e366251bd9c63005241775f672bf3ec

Thanks!

-- 
MST

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

[Peter Maydell]

On 3 November 2014 16:35, Michael S. Tsirkin <mst@redhat.com> wrote:
> OK I backed that patch out and re-pushed.
> As other patches are unchanged, I don't want to re-do the pull
> request, it's just spam on list.
>
> Tag name is still for_upstream, new hash
> d43f0d641e366251bd9c63005241775f672bf3ec

OK, thanks; I've applied this. You can always just send
the cover letter again in this sort of situation, by the way.
(PS: if you can change either the commit message or the tag
name on a respin to indicate which version of a pullreq it is
it helps me be certain I have the right one.)

thanks
-- PMM

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

[Michael S. Tsirkin]

On Tue, Nov 04, 2014 at 12:17:29AM +0000, Peter Maydell wrote:
> On 3 November 2014 16:35, Michael S. Tsirkin <mst@redhat.com> wrote:
> > OK I backed that patch out and re-pushed.
> > As other patches are unchanged, I don't want to re-do the pull
> > request, it's just spam on list.
> >
> > Tag name is still for_upstream, new hash
> > d43f0d641e366251bd9c63005241775f672bf3ec
> 
> OK, thanks; I've applied this. You can always just send
> the cover letter again in this sort of situation, by the way.
> (PS: if you can change either the commit message or the tag
> name on a respin to indicate which version of a pullreq it is
> it helps me be certain I have the right one.)
> 
> thanks
> -- PMM

OK, will do that if this happens again.

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

[Michael S. Tsirkin]

On Mon, Nov 03, 2014 at 05:13:15PM +0100, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > 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 :(
> 
> Oh, now I see: you already sent this in a pull request, after less than
> 20 hours on list.  Please don't do that except in dire emergency.  It
> sabotages our review process.

I sometimes do this for patches that seem trivial to me.
I've backed this one out for now.

-- 
MST

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

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Mon, Nov 03, 2014 at 05:13:15PM +0100, Markus Armbruster wrote:
>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>> 
>> > 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 :(
>> 
>> Oh, now I see: you already sent this in a pull request, after less than
>> 20 hours on list.  Please don't do that except in dire emergency.  It
>> sabotages our review process.
>
> I sometimes do this for patches that seem trivial to me.

Please don't.  No patch is too trivial for review.

> I've backed this one out for now.

Thanks.

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> "Michael S. Tsirkin" <mst@redhat.com> writes:
>
>> On Mon, Nov 03, 2014 at 05:13:15PM +0100, Markus Armbruster wrote:
>>> "Michael S. Tsirkin" <mst@redhat.com> writes:
>>> 
>>> > 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 :(
>>> 
>>> Oh, now I see: you already sent this in a pull request, after less than
>>> 20 hours on list.  Please don't do that except in dire emergency.  It
>>> sabotages our review process.
>>
>> I sometimes do this for patches that seem trivial to me.
>
> Please don't.  No patch is too trivial for review.
>
>> I've backed this one out for now.
>
> Thanks.

I just noticed you simply resent the thing unchanged in your next pull
request.  Pray tell me, why should I spend my time on reviewing your
patches?


Timeline:

Nov 2 You submit the patch
Nov 3 You include it in a pull req after <20h
Nov 3 I review, ask for minor improvements
      You reply "too late"
      Peter asks you to drop it
      You comply
Nov 23 You include it in a pull req *unchanged*, doesn't build
Nov 24 You include it in a pull req *unchanged*, Peter applies