[SRU,Bionic] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE
diff mbox series

Message ID 20190826171418.24238-1-kamal@canonical.com
State New
Headers show
Series
  • [SRU,Bionic] tuntap: correctly set SOCKWQ_ASYNC_NOSPACE
Related show

Commit Message

Kamal Mostafa Aug. 26, 2019, 5:14 p.m. UTC
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(-)

Comments

Stefan Bader Aug. 27, 2019, 3:04 p.m. UTC | #1
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>
Nicolas Dichtel Aug. 28, 2019, 1:48 p.m. UTC | #2
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
Stefan Bader Aug. 28, 2019, 3:24 p.m. UTC | #3
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
>
Nicolas Dichtel Aug. 29, 2019, 2:43 p.m. UTC | #4
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
Kleber Souza Sept. 2, 2019, 4:26 p.m. UTC | #5
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)
>
Kleber Souza Sept. 3, 2019, 4:23 p.m. UTC | #6
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

Patch
diff mbox series

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)