On 2018年05月22日 11:46, Michael S. Tsirkin wrote:
On Tue, May 22, 2018 at 11:22:11AM +0800, Jason Wang wrote:
On 2018年05月22日 06:08, Michael S. Tsirkin wrote:
On Mon, May 21, 2018 at 11:47:42AM -0400, David Miller wrote:
From: Jason Wang <jasow...@redhat.com>
Date: Fri, 18 May 2018 21:00:43 +0800
We return -EIO on device down but can not raise EPOLLOUT after it was
up. This may confuse user like vhost which expects tuntap to raise
EPOLLOUT to re-enable its TX routine after tuntap is down. This could
be easily reproduced by transmitting packets from VM while down and up
the tap device. Fixing this by set SOCKWQ_ASYNC_NOSPACE on -EIO.
Cc: Hannes Frederic Sowa <han...@stressinduktion.org>
Cc: Eric Dumazet <eduma...@google.com>
Fixes: 1bd4978a88ac2 ("tun: honor IFF_UP in tun_get_user()")
Signed-off-by: Jason Wang <jasow...@redhat.com>
I'm no so sure what to do with this patch.
Like Michael says, this flag bit is only checks upon transmit which
may or may not happen after this point. It doesn't seem to be
guaranteed.
The flag is checked in tun_chr_poll() as well.
Jason, can't we detect a link up transition and respond accordingly?
What do you think?
I think we've already tried to do this, in tun_net_open() we call
write_space(). But the problem is the bit may not be set at that time.
A second thought is to set the bit in tun_chr_poll() instead of -EIO like:
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index d45ac37..46a1573 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -1423,6 +1423,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 */
@@ -1445,10 +1452,9 @@ static __poll_t tun_chr_poll(struct file *file,
poll_table *wait)
if (!ptr_ring_empty(&tfile->tx_ring))
mask |= EPOLLIN | EPOLLRDNORM;
- if (tun->dev->flags & IFF_UP &&
- (sock_writeable(sk) ||
- (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags)
&&
- sock_writeable(sk))))
+ if (tun_sock_writeable(tun, tfile) ||
+ (!test_and_set_bit(SOCKWQ_ASYNC_NOSPACE, &sk->sk_socket->flags)
&&
+ tun_sock_writeable(tun, tfile)));
mask |= EPOLLOUT | EPOLLWRNORM;
if (tun->dev->reg_state != NETREG_REGISTERED)
Does this make more sense?
Thanks
I just understood the motivation for doing it on EIO.
Maybe there's a reason it makes sense here as well,
but it's far from obvious. I suggest you repost adding
an explanation in the comment. The original patch will
be fine with an explanation as well.
Ok, let me add explanation on both code and commit log.
Thanks
