| Message ID | 20190826171418.24238-1-kamal@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [SRU,Bionic] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE | expand |
On 26.08.19 19:14, Kamal Mostafa wrote: > 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()") Normally bug reports for SRU should have a justification section like I have added now for this one. Ideally with a test case (or steps how something can be verified). If someone could do the same for the ipv6 neighbour resolution request. Thanks Acked-by: Stefan Bader <stefan.bader@canonical.com>
Le 27/08/2019 à 17:04, Stefan Bader a écrit : > On 26.08.19 19:14, Kamal Mostafa wrote: >> 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()") > > Normally bug reports for SRU should have a justification section like I have > added now for this one. Ideally with a test case (or steps how something can be > verified). If someone could do the same for the ipv6 neighbour resolution > request. Thanks The description and the test case are explained in the commit log, it's just a copy and paste. But what do you expect in the "Fix" section? For now, the fix has only been backported in disco. I was checking bionic but not xenial 4.4. Regards, Nicolas
On 28.08.19 15:48, Nicolas Dichtel wrote: > Le 27/08/2019 à 17:04, Stefan Bader a écrit : >> On 26.08.19 19:14, Kamal Mostafa wrote: >>> 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()") >> >> Normally bug reports for SRU should have a justification section like I have >> added now for this one. Ideally with a test case (or steps how something can be >> verified). If someone could do the same for the ipv6 neighbour resolution >> request. Thanks > > The description and the test case are explained in the commit log, it's just a > copy and paste. But what do you expect in the "Fix" section? > For now, the fix has only been backported in disco. I was checking bionic but > not xenial 4.4. Fix usually is just stating what is required to fix the issue. Here just the patch you propose. The whole justification is coming from general stable release update procedures and is geared towards people which are not necessarily kernel people. In the case at hand I would probably add more details which kernel version the fix comes from and which kernel versions potentially are needing this. (Introduced with ... , maybe/likely 4.4 got it via some upstream stable) -Stefan > > > Regards, > Nicolas >
Le 28/08/2019 à 17:24, Stefan Bader a écrit : > On 28.08.19 15:48, Nicolas Dichtel wrote: >> Le 27/08/2019 à 17:04, Stefan Bader a écrit : >>> On 26.08.19 19:14, Kamal Mostafa wrote: >>>> 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()") >>> >>> Normally bug reports for SRU should have a justification section like I have >>> added now for this one. Ideally with a test case (or steps how something can be >>> verified). If someone could do the same for the ipv6 neighbour resolution >>> request. Thanks >> >> The description and the test case are explained in the commit log, it's just a >> copy and paste. But what do you expect in the "Fix" section? >> For now, the fix has only been backported in disco. I was checking bionic but >> not xenial 4.4. > > Fix usually is just stating what is required to fix the issue. Here just the > patch you propose. The whole justification is coming from general stable release > update procedures and is geared towards people which are not necessarily kernel > people. In the case at hand I would probably add more details which kernel > version the fix comes from and which kernel versions potentially are needing > this. (Introduced with ... , maybe/likely 4.4 got it via some upstream stable) I've added this SRU: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1834465 Feel free to update it if needed. Regards, Nicolas
On 8/26/19 7:14 PM, Kamal Mostafa wrote: > From: Jason Wang <jasowang@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1830756 > > When link is down, writes to the device might fail with > -EIO. Userspace needs an indication when the status is resolved. As a > fix, tun_net_open() attempts to wake up writers - but that is only > effective if SOCKWQ_ASYNC_NOSPACE has been set in the past. This is > not the case of vhost_net which only poll for EPOLLOUT after it meets > errors during sendmsg(). > > This patch fixes this by making sure SOCKWQ_ASYNC_NOSPACE is set when > socket is not writable or device is down to guarantee EPOLLOUT will be > raised in either tun_chr_poll() or tun_sock_write_space() after device > is up. > > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> > Cc: Eric Dumazet <edumazet@google.com> > Fixes: 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()") > Signed-off-by: Jason Wang <jasowang@redhat.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (backported from commit 2f3ab6221e4c87960347d65c7cab9bd917d1f637) > Signed-off-by: Kamal Mostafa <kamal@canonical.com> Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> > --- > drivers/net/tun.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 01c706f8fb74..5d1d5bf3a378 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1252,6 +1252,13 @@ static void tun_net_init(struct net_device *dev) > dev->max_mtu = MAX_MTU - dev->hard_header_len; > } > > +static bool tun_sock_writeable(struct tun_struct *tun, struct tun_file *tfile) > +{ > + struct sock *sk = tfile->socket.sk; > + > + return (tun->dev->flags & IFF_UP) && sock_writeable(sk); > +} > + > /* Character device part */ > > /* Poll */ > @@ -1274,10 +1281,14 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait) > if (!skb_array_empty(&tfile->tx_array)) > mask |= POLLIN | POLLRDNORM; > > - if (tun->dev->flags & IFF_UP && > - (sock_writeable(sk) || > - (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags) && > - sock_writeable(sk)))) > + /* Make sure SOCKWQ_ASYNC_NOSPACE is set if not writable to > + * guarantee EPOLLOUT to be raised by either here or > + * tun_sock_write_space(). Then process could get notification > + * after it writes to a down device and meets -EIO. > + */ > + if (tun_sock_writeable(tun, tfile) || > + (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags) && > + tun_sock_writeable(tun, tfile))) > mask |= POLLOUT | POLLWRNORM; > > if (tun->dev->reg_state != NETREG_REGISTERED) >
On 8/26/19 7:14 PM, Kamal Mostafa wrote: > From: Jason Wang <jasowang@redhat.com> > > BugLink: https://bugs.launchpad.net/bugs/1830756 > > When link is down, writes to the device might fail with > -EIO. Userspace needs an indication when the status is resolved. As a > fix, tun_net_open() attempts to wake up writers - but that is only > effective if SOCKWQ_ASYNC_NOSPACE has been set in the past. This is > not the case of vhost_net which only poll for EPOLLOUT after it meets > errors during sendmsg(). > > This patch fixes this by making sure SOCKWQ_ASYNC_NOSPACE is set when > socket is not writable or device is down to guarantee EPOLLOUT will be > raised in either tun_chr_poll() or tun_sock_write_space() after device > is up. > > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org> > Cc: Eric Dumazet <edumazet@google.com> > Fixes: 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()") > Signed-off-by: Jason Wang <jasowang@redhat.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > (backported from commit 2f3ab6221e4c87960347d65c7cab9bd917d1f637) > Signed-off-by: Kamal Mostafa <kamal@canonical.com> > --- > drivers/net/tun.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 01c706f8fb74..5d1d5bf3a378 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -1252,6 +1252,13 @@ static void tun_net_init(struct net_device *dev) > dev->max_mtu = MAX_MTU - dev->hard_header_len; > } > > +static bool tun_sock_writeable(struct tun_struct *tun, struct tun_file *tfile) > +{ > + struct sock *sk = tfile->socket.sk; > + > + return (tun->dev->flags & IFF_UP) && sock_writeable(sk); > +} > + > /* Character device part */ > > /* Poll */ > @@ -1274,10 +1281,14 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait) > if (!skb_array_empty(&tfile->tx_array)) > mask |= POLLIN | POLLRDNORM; > > - if (tun->dev->flags & IFF_UP && > - (sock_writeable(sk) || > - (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags) && > - sock_writeable(sk)))) > + /* Make sure SOCKWQ_ASYNC_NOSPACE is set if not writable to > + * guarantee EPOLLOUT to be raised by either here or > + * tun_sock_write_space(). Then process could get notification > + * after it writes to a down device and meets -EIO. > + */ > + if (tun_sock_writeable(tun, tfile) || > + (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags) && > + tun_sock_writeable(tun, tfile))) > mask |= POLLOUT | POLLWRNORM; > > if (tun->dev->reg_state != NETREG_REGISTERED) > Applied to bionic/master-next branch. Thanks, Kleber
diff --git a/drivers/net/tun.c b/drivers/net/tun.c index 01c706f8fb74..5d1d5bf3a378 100644 --- a/drivers/net/tun.c +++ b/drivers/net/tun.c @@ -1252,6 +1252,13 @@ static void tun_net_init(struct net_device *dev) dev->max_mtu = MAX_MTU - dev->hard_header_len; } +static bool tun_sock_writeable(struct tun_struct *tun, struct tun_file *tfile) +{ + struct sock *sk = tfile->socket.sk; + + return (tun->dev->flags & IFF_UP) && sock_writeable(sk); +} + /* Character device part */ /* Poll */ @@ -1274,10 +1281,14 @@ static unsigned int tun_chr_poll(struct file *file, poll_table *wait) if (!skb_array_empty(&tfile->tx_array)) mask |= POLLIN | POLLRDNORM; - if (tun->dev->flags & IFF_UP && - (sock_writeable(sk) || - (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags) && - sock_writeable(sk)))) + /* Make sure SOCKWQ_ASYNC_NOSPACE is set if not writable to + * guarantee EPOLLOUT to be raised by either here or + * tun_sock_write_space(). Then process could get notification + * after it writes to a down device and meets -EIO. + */ + if (tun_sock_writeable(tun, tfile) || + (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags) && + tun_sock_writeable(tun, tfile))) mask |= POLLOUT | POLLWRNORM; if (tun->dev->reg_state != NETREG_REGISTERED)