[Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[linzhecheng]

Signed-off-by: linzhecheng <linzhecheng@huawei.com>

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 159e69c3b1..17519ec589 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
                                         s->write_msgfds,
                                         s->write_msgfds_num);
 
-        /* free the written msgfds, no matter what */
-        if (s->write_msgfds_num) {
+        /* free the written msgfds in any cases other than errno==EAGAIN */
+        if (EAGAIN != errno && s->write_msgfds_num) {
             g_free(s->write_msgfds);
             s->write_msgfds = 0;
             s->write_msgfds_num = 0;
-- 
2.12.2.windows.2

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Marc-André Lureau]

Hi

On Tue, May 29, 2018 at 4:52 AM, linzhecheng <linzhecheng@huawei.com> wrote:
> Signed-off-by: linzhecheng <linzhecheng@huawei.com>
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..17519ec589 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
>
> -        /* free the written msgfds, no matter what */
> -        if (s->write_msgfds_num) {
> +        /* free the written msgfds in any cases other than errno==EAGAIN */
> +        if (EAGAIN != errno && s->write_msgfds_num) {
>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;
> --

It is already fix since v2.12:

commit c863fdec6aff6b5a4ca8fff1537b80d9f8b97726
Author: Daniel P. Berrangé <berrange@redhat.com>
Date:   Thu Feb 22 12:13:51 2018 +0000

    chardev: fix handling of EAGAIN for TCP chardev

> 2.12.2.windows.2
>
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Peter Xu]

On Tue, May 29, 2018 at 10:52:40AM +0800, linzhecheng wrote:
> Signed-off-by: linzhecheng <linzhecheng@huawei.com>
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..17519ec589 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
>  
> -        /* free the written msgfds, no matter what */
> -        if (s->write_msgfds_num) {
> +        /* free the written msgfds in any cases other than errno==EAGAIN */
> +        if (EAGAIN != errno && s->write_msgfds_num) {

This makes sense to me.

Meanwhile this reminds me that we didn't release the fds in
tcp_chr_disconnect().  I'm thinking whether we should do that no
matter what.  E.g., what if we set_msgfds() then another operation
instead of write() failed?

Then here we can only release the msgfds when write succeeded...

  if (ret >= 0 && s->write_msgfds_num) {
 
...as long as we'll also cleanup the msgfds in tcp_chr_disconnect()
always.

I'll see how Marc-Andre and Paolo see this though.

Thanks,

>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;
> -- 
> 2.12.2.windows.2
> 
> 
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Daniel P. Berrangé]

On Tue, May 29, 2018 at 10:52:40AM +0800, linzhecheng wrote:
> Signed-off-by: linzhecheng <linzhecheng@huawei.com>
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..17519ec589 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
>  
> -        /* free the written msgfds, no matter what */
> -        if (s->write_msgfds_num) {
> +        /* free the written msgfds in any cases other than errno==EAGAIN */
> +        if (EAGAIN != errno && s->write_msgfds_num) {
>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;

Thanks, queued with an updated commit message

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[linzhecheng]

CC'ing Daniel P. Berrangé , Peter Xu, Marc-André Lureau, Eric Blake, Gonglei

> -----邮件原件-----
> 发件人: linzhecheng
> 发送时间: 2018年5月29日 10:53
> 收件人: qemu-devel@nongnu.org
> 抄送: pbonzini@redhat.com; wangxin (U) <wangxinxin.wang@huawei.com>;
> linzhecheng <linzhecheng@huawei.com>
> 主题: [PATCH] socket: dont't free msgfds if error equals EAGAIN
> 
> Signed-off-by: linzhecheng <linzhecheng@huawei.com>
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c index
> 159e69c3b1..17519ec589 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t
> *buf, int len)
>                                          s->write_msgfds,
>                                          s->write_msgfds_num);
> 
> -        /* free the written msgfds, no matter what */
> -        if (s->write_msgfds_num) {
> +        /* free the written msgfds in any cases other than errno==EAGAIN */
> +        if (EAGAIN != errno && s->write_msgfds_num) {
>              g_free(s->write_msgfds);
>              s->write_msgfds = 0;
>              s->write_msgfds_num = 0;
> --
> 2.12.2.windows.2
> 

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Gonglei (Arei)]

Hi all,

The issue is easy to reproduce when we confiugred multi-queue function for vhost-user nics.

The main backtrace is as follows:

vhost_user_write  ==>  0)  sets s->write_msgfds_num to 8
	qemu_chr_fe_write_all
		qemu_chr_fe_write_buffer  ==> 4) rewrite because (ret <0 && errno is EAGAIN)
			tcp_chr_write  ==> 3) clear resource about s->write_msgfds and set s->write_msgfds_num to 0
				io_channel_send_full  ==>  2) errno = EAGAIN and return -1
					qio_channel_socket_writev  ==> 1) returns QIO_CHANNEL_ERR_BLOCK when ret <0 && errno == EAGAIN

Then at the above step 4) may cause undefined behaviors on the vhost-user server side because null control message is sent. 

So, we submit a patch to fix it. What's your opinion?

Regards,
-Gonglei

> -----Original Message-----
> From: linzhecheng
> Sent: Tuesday, May 29, 2018 4:20 PM
> To: qemu-devel@nongnu.org
> Cc: pbonzini@redhat.com; wangxin (U) <wangxinxin.wang@huawei.com>;
> berrange@redhat.com; peterx@redhat.com; marcandre.lureau@redhat.com;
> eblake@redhat.com; Gonglei (Arei) <arei.gonglei@huawei.com>
> Subject: RE: [PATCH] socket: dont't free msgfds if error equals EAGAIN
> 
> CC'ing Daniel P. Berrangé , Peter Xu, Marc-André Lureau, Eric Blake, Gonglei
> 
> > -----邮件原件-----
> > 发件人: linzhecheng
> > 发送时间: 2018年5月29日 10:53
> > 收件人: qemu-devel@nongnu.org
> > 抄送: pbonzini@redhat.com; wangxin (U)
> <wangxinxin.wang@huawei.com>;
> > linzhecheng <linzhecheng@huawei.com>
> > 主题: [PATCH] socket: dont't free msgfds if error equals EAGAIN
> >
> > Signed-off-by: linzhecheng <linzhecheng@huawei.com>
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c index
> > 159e69c3b1..17519ec589 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t
> > *buf, int len)
> >                                          s->write_msgfds,
> >                                          s->write_msgfds_num);
> >
> > -        /* free the written msgfds, no matter what */
> > -        if (s->write_msgfds_num) {
> > +        /* free the written msgfds in any cases other than errno==EAGAIN
> */
> > +        if (EAGAIN != errno && s->write_msgfds_num) {
> >              g_free(s->write_msgfds);
> >              s->write_msgfds = 0;
> >              s->write_msgfds_num = 0;
> > --
> > 2.12.2.windows.2
> >

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[linzhecheng]

I think this patch doesn't fix my issue. For more details, please see Gonglei's reply.
https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06296.html

> -----邮件原件-----
> 发件人: Marc-André Lureau [mailto:marcandre.lureau@gmail.com]
> 发送时间: 2018年5月29日 17:11
> 收件人: linzhecheng <linzhecheng@huawei.com>
> 抄送: QEMU <qemu-devel@nongnu.org>; Paolo Bonzini
> <pbonzini@redhat.com>; wangxin (U) <wangxinxin.wang@huawei.com>
> 主题: Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals
> EAGAIN
> 
> Hi
> 
> On Tue, May 29, 2018 at 4:52 AM, linzhecheng <linzhecheng@huawei.com>
> wrote:
> > Signed-off-by: linzhecheng <linzhecheng@huawei.com>
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c index
> > 159e69c3b1..17519ec589 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -134,8 +134,8 @@ static int tcp_chr_write(Chardev *chr, const uint8_t
> *buf, int len)
> >                                          s->write_msgfds,
> >                                          s->write_msgfds_num);
> >
> > -        /* free the written msgfds, no matter what */
> > -        if (s->write_msgfds_num) {
> > +        /* free the written msgfds in any cases other than errno==EAGAIN */
> > +        if (EAGAIN != errno && s->write_msgfds_num) {
> >              g_free(s->write_msgfds);
> >              s->write_msgfds = 0;
> >              s->write_msgfds_num = 0;
> > --
> 
> It is already fix since v2.12:
> 
> commit c863fdec6aff6b5a4ca8fff1537b80d9f8b97726
> Author: Daniel P. Berrangé <berrange@redhat.com>
> Date:   Thu Feb 22 12:13:51 2018 +0000
> 
>     chardev: fix handling of EAGAIN for TCP chardev
> 
> > 2.12.2.windows.2
> >
> >
> >
> 
> 
> 
> --
> Marc-André Lureau

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Eric Blake]

On 05/29/2018 04:33 AM, linzhecheng wrote:
> I think this patch doesn't fix my issue. For more details, please see Gonglei's reply.
> https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06296.html

Your mailer is not honoring threading (it failed to include 
'In-Reply-To:' and 'References:' headers that refer to the message you 
are replying to), and you are top-posting, both of which make it 
difficult to follow your comments on a technical list.


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals EAGAIN

[Gonglei (Arei)]

> -----Original Message-----
> From: Eric Blake [mailto:eblake@redhat.com]
> Sent: Wednesday, May 30, 2018 3:33 AM
> To: linzhecheng <linzhecheng@huawei.com>; Marc-André Lureau
> <marcandre.lureau@gmail.com>
> Cc: QEMU <qemu-devel@nongnu.org>; Paolo Bonzini <pbonzini@redhat.com>;
> wangxin (U) <wangxinxin.wang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; peterx@redhat.com; berrange@redhat.com
> Subject: Re: [Qemu-devel] [PATCH] socket: dont't free msgfds if error equals
> EAGAIN
> 
> On 05/29/2018 04:33 AM, linzhecheng wrote:
> > I think this patch doesn't fix my issue. For more details, please see Gonglei's
> reply.
> > https://lists.gnu.org/archive/html/qemu-devel/2018-05/msg06296.html
> 
> Your mailer is not honoring threading (it failed to include
> 'In-Reply-To:' and 'References:' headers that refer to the message you
> are replying to), and you are top-posting, both of which make it
> difficult to follow your comments on a technical list.
> 
> 
Agree.

@Zhecheng, pls resend a patch with commit message. Ccing these guys.

Regards,
-Gonglei