tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Nicolas Dichtel
Hi Ubuntu kernel team,

please, can you consider to cherry-pick the following linux upstream fix:
2f3ab6221e4c ("tuntap: correctly set SOCKWQ_ASYNC_NOSPACE").
It has been included in v4.17 and the patch that introduces the bug has been
included in v4.5.

Here is the launchpad entry:
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1830756


Thank you,
Nicolas

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Kamal Mostafa-2
This commit requires a light backport, which I will submit to our bionic
patch pipeline.  Thanks for the recommendation, Nicolas!

 -Kamal

On Tue, Aug 13, 2019 at 10:42:44AM +0200, Nicolas Dichtel wrote:

> Hi Ubuntu kernel team,
>
> please, can you consider to cherry-pick the following linux upstream fix:
> 2f3ab6221e4c ("tuntap: correctly set SOCKWQ_ASYNC_NOSPACE").
> It has been included in v4.17 and the patch that introduces the bug has been
> included in v4.5.
>
> Here is the launchpad entry:
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1830756
>
>
> Thank you,
> Nicolas
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Kamal Mostafa-2
In reply to this post by Nicolas Dichtel
From: Jason Wang <[hidden email]>

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 <[hidden email]>
Cc: Eric Dumazet <[hidden email]>
Fixes: 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()")
Signed-off-by: Jason Wang <[hidden email]>
Signed-off-by: David S. Miller <[hidden email]>
(backported from commit 2f3ab6221e4c87960347d65c7cab9bd917d1f637)
Signed-off-by: Kamal Mostafa <[hidden email]>
---
 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)
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK/Cmnt: [SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Stefan Bader-2
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 <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/Cmnt: [SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

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

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

Re: ACK/Cmnt: [SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Stefan Bader-2
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
>



--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: ACK/Cmnt: [SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

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

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Kleber Souza
In reply to this post by Kamal Mostafa-2
On 8/26/19 7:14 PM, Kamal Mostafa wrote:

> From: Jason Wang <[hidden email]>
>
> 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 <[hidden email]>
> Cc: Eric Dumazet <[hidden email]>
> Fixes: 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()")
> Signed-off-by: Jason Wang <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (backported from commit 2f3ab6221e4c87960347d65c7cab9bd917d1f637)
> Signed-off-by: Kamal Mostafa <[hidden email]>

Acked-by: Kleber Sacilotto de Souza <[hidden email]>

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


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU][Bionic][PATCH] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE

Kleber Souza
In reply to this post by Kamal Mostafa-2
On 8/26/19 7:14 PM, Kamal Mostafa wrote:

> From: Jason Wang <[hidden email]>
>
> 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 <[hidden email]>
> Cc: Eric Dumazet <[hidden email]>
> Fixes: 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()")
> Signed-off-by: Jason Wang <[hidden email]>
> Signed-off-by: David S. Miller <[hidden email]>
> (backported from commit 2f3ab6221e4c87960347d65c7cab9bd917d1f637)
> Signed-off-by: Kamal Mostafa <[hidden email]>
> ---
>  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

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team