qemu-char: fix tcp_get_fds

PATCH In review Michael S. Tsirkin 3 participants 13 messages Nov 2, 2014 – Nov 27, 2014

lore

Share & tools

lore

Michael S. Tsirkin

author

2014-11-02 16:54 UTC #0 lore raw

PATCH 1/1 qemu-char: fix tcp_get_fds

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 <[email protected]>

---
 qemu-char.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

qemu-char.c +4 -1

index bd0709b..1c4004c 100644
@@ -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;

Signature · 1 line

MST

Markus Armbruster

Reviewed-by on v1 1/1

2014-11-03 15:09 UTC #1 lore raw

Quoted 1 line · Michael S. Tsirkin

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?

Quoted 27 lines

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 <[email protected]>
---
 qemu-char.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
qemu-char.c +1 -0
index bd0709b..1c4004c 100644
@@ -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 */

qemu-char.c static int tcp_get_msgfds(CharDriverState *chr, int *fds, int num) · line 2676

Quoted 8 lines

qemu-char.c +2 -0

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

qemu-char.c static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len) · line 2771

Quoted 9 lines

qemu-char.c +1 -1

@@ -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 <[email protected]>

Michael S. Tsirkin

author

2014-11-03 15:20 UTC #2 lore raw

Quoted 5 lines · Markus Armbruster

Quoted 1 line · Michael S. Tsirkin
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 :(

Quoted 1 line

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.

Quoted 52 lines

Quoted 27 lines
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 <[email protected]>
---
 qemu-char.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
qemu-char.c +1 -0
index bd0709b..1c4004c 100644
@@ -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 */
qemu-char.c static int tcp_get_msgfds(CharDriverState *chr, int *fds, int num) · line 2676
Quoted 8 lines
qemu-char.c +2 -0
@@ -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.
qemu-char.c static ssize_t tcp_chr_recv(CharDriverState *chr, char *buf, size_t len) · line 2771
Quoted 9 lines
qemu-char.c +1 -1
@@ -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 <[email protected]>

Markus Armbruster

2014-11-03 16:07 UTC #3 lore raw

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
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?

Quoted 4 lines

Quoted 1 line
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.

Quoted 6 lines

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

[...]

Markus Armbruster

2014-11-03 16:13 UTC #4 lore raw

In reply to Michael S. Tsirkin

Quoted 8 lines · Michael S. Tsirkin

Quoted 5 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
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.

Peter Maydell

2014-11-03 16:23 UTC #5 lore raw

Quoted 7 lines · Markus Armbruster

Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
tcp_get_fds API discards fds if there's more than 1 of these.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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

Michael S. Tsirkin

author

2014-11-03 16:32 UTC #6 lore raw

Quoted 16 lines · Peter Maydell

Quoted 7 lines · Markus Armbruster
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
tcp_get_fds API discards fds if there's more than 1 of these.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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.

Quoted 7 lines

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

Michael S. Tsirkin

author

2014-11-03 16:35 UTC #7 lore raw

In reply to Peter Maydell

Quoted 16 lines · Peter Maydell

Quoted 7 lines · Markus Armbruster
Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
tcp_get_fds API discards fds if there's more than 1 of these.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
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.

Quoted 7 lines

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

Signature · 1 line

MST

Peter Maydell

2014-11-04 00:18 UTC #8 lore raw

Quoted 6 lines · Michael S. Tsirkin

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

Michael S. Tsirkin

author

2014-11-04 13:49 UTC #9 lore raw

Quoted 8 lines · Peter Maydell

Quoted 6 lines · Michael S. Tsirkin
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

Quoted 8 lines

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.

Michael S. Tsirkin

author

2014-11-03 16:40 UTC #10 lore raw

In reply to Markus Armbruster

Quoted 7 lines · Markus Armbruster

Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
tcp_get_fds API discards fds if there's more than 1 of these.

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
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.

Signature · 1 line

MST

Markus Armbruster

2014-11-04 06:51 UTC #11 lore raw

Quoted 10 lines · Michael S. Tsirkin

Quoted 9 lines · Markus Armbruster
Quoted 7 lines · Michael S. Tsirkin
Quoted 5 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
tcp_get_fds API discards fds if there's more than 1 of these.
s/tcp_get_fds/tcp_get_msgfds/ (subject as well)

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
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.

Quoted 1 line

I've backed this one out for now.

Thanks.

Markus Armbruster

2014-11-27 09:58 UTC #12 lore raw

Quoted 18 lines · Markus Armbruster

Quoted 16 lines · Michael S. Tsirkin
Quoted 14 lines · Markus Armbruster
Quoted 8 lines · Michael S. Tsirkin
Quoted 5 lines · Markus Armbruster
Quoted 1 line · Michael S. Tsirkin
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.

Quoted 7 lines

Quoted 1 line
I sometimes do this for patches that seem trivial to me.
Please don't. No patch is too trivial for review.
Quoted 1 line
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