From: Jan Kiszka <jan.kiszka@siemens.com> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: "David S. Miller" <davem@davemloft.net>, Herbert Xu <herbert@gondor.hengli.com.au>, Paul Moore <paul.moore@hp.com>, David Woodhouse <David.Woodhouse@intel.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, qemu-devel <qemu-devel@nongnu.org> Subject: Re: [PATCH] tun: orphan an skb on tx Date: Tue, 13 Apr 2010 17:36:25 +0200 [thread overview] Message-ID: <4BC48F79.5090409@siemens.com> (raw) In-Reply-To: <20100413145944.GA7716@redhat.com> Michael S. Tsirkin wrote: > The following situation was observed in the field: > tap1 sends packets, tap2 does not consume them, as a result > tap1 can not be closed. And before that, tap1 may not be able to send further packets to anyone else on the bridge as its TX resources were blocked by tap2 - that's what we saw in the field. > This happens because > tun/tap devices can hang on to skbs undefinitely. > > As noted by Herbert, possible solutions include a timeout followed by a > copy/change of ownership of the skb, or always copying/changing > ownership if we're going into a hostile device. > > This patch implements the second approach. > > Note: one issue still remaining is that since skbs > keep reference to tun socket and tun socket has a > reference to tun device, we won't flush backlog, > instead simply waiting for all skbs to get transmitted. > At least this is not user-triggerable, and > this was not reported in practice, my assumption is > other devices besides tap complete an skb > within finite time after it has been queued. > > A possible solution for the second issue > would not to have socket reference the device, > instead, implement dev->destructor for tun, and > wait for all skbs to complete there, but this > needs some thought, probably too risky for 2.6.34. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Tested-by: Yan Vugenfirer <yvugenfi@redhat.com> > > --- > > Please review the below, and consider for 2.6.34, > and stable trees. > > drivers/net/tun.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 96c39bd..4326520 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -387,6 +387,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > } > } > > + /* Orphan the skb - required as we might hang on to it > + * for indefinite time. */ > + skb_orphan(skb); > + > /* Enqueue packet */ > skb_queue_tail(&tun->socket.sk->sk_receive_queue, skb); > dev->trans_start = jiffies; Nice solution, thanks! Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
WARNING: multiple messages have this Message-ID (diff)
From: Jan Kiszka <jan.kiszka@siemens.com> To: "Michael S. Tsirkin" <mst@redhat.com> Cc: Paul Moore <paul.moore@hp.com>, David Woodhouse <David.Woodhouse@intel.com>, "netdev@vger.kernel.org" <netdev@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, qemu-devel <qemu-devel@nongnu.org>, Herbert Xu <herbert@gondor.hengli.com.au>, "David S. Miller" <davem@davemloft.net> Subject: [Qemu-devel] Re: [PATCH] tun: orphan an skb on tx Date: Tue, 13 Apr 2010 17:36:25 +0200 [thread overview] Message-ID: <4BC48F79.5090409@siemens.com> (raw) In-Reply-To: <20100413145944.GA7716@redhat.com> Michael S. Tsirkin wrote: > The following situation was observed in the field: > tap1 sends packets, tap2 does not consume them, as a result > tap1 can not be closed. And before that, tap1 may not be able to send further packets to anyone else on the bridge as its TX resources were blocked by tap2 - that's what we saw in the field. > This happens because > tun/tap devices can hang on to skbs undefinitely. > > As noted by Herbert, possible solutions include a timeout followed by a > copy/change of ownership of the skb, or always copying/changing > ownership if we're going into a hostile device. > > This patch implements the second approach. > > Note: one issue still remaining is that since skbs > keep reference to tun socket and tun socket has a > reference to tun device, we won't flush backlog, > instead simply waiting for all skbs to get transmitted. > At least this is not user-triggerable, and > this was not reported in practice, my assumption is > other devices besides tap complete an skb > within finite time after it has been queued. > > A possible solution for the second issue > would not to have socket reference the device, > instead, implement dev->destructor for tun, and > wait for all skbs to complete there, but this > needs some thought, probably too risky for 2.6.34. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Tested-by: Yan Vugenfirer <yvugenfi@redhat.com> > > --- > > Please review the below, and consider for 2.6.34, > and stable trees. > > drivers/net/tun.c | 4 ++++ > 1 files changed, 4 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c > index 96c39bd..4326520 100644 > --- a/drivers/net/tun.c > +++ b/drivers/net/tun.c > @@ -387,6 +387,10 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev) > } > } > > + /* Orphan the skb - required as we might hang on to it > + * for indefinite time. */ > + skb_orphan(skb); > + > /* Enqueue packet */ > skb_queue_tail(&tun->socket.sk->sk_receive_queue, skb); > dev->trans_start = jiffies; Nice solution, thanks! Jan -- Siemens AG, Corporate Technology, CT T DE IT 1 Corporate Competence Center Embedded Linux
next prev parent reply other threads:[~2010-04-13 15:37 UTC|newest] Thread overview: 64+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-04-13 14:59 [PATCH] tun: orphan an skb on tx Michael S. Tsirkin 2010-04-13 14:59 ` [Qemu-devel] " Michael S. Tsirkin 2010-04-13 15:12 ` Herbert Xu 2010-04-13 15:12 ` [Qemu-devel] " Herbert Xu 2010-04-13 15:12 ` Herbert Xu 2010-04-13 15:36 ` Jan Kiszka [this message] 2010-04-13 15:36 ` [Qemu-devel] " Jan Kiszka 2010-04-13 16:40 ` Eric Dumazet 2010-04-13 16:40 ` [Qemu-devel] " Eric Dumazet 2010-04-13 16:52 ` Jan Kiszka 2010-04-13 16:52 ` [Qemu-devel] " Jan Kiszka 2010-04-13 17:39 ` Michael S. Tsirkin 2010-04-13 17:39 ` [Qemu-devel] " Michael S. Tsirkin 2010-04-13 18:31 ` Eric Dumazet 2010-04-13 18:31 ` [Qemu-devel] " Eric Dumazet 2010-04-13 20:25 ` Michael S. Tsirkin 2010-04-13 20:25 ` [Qemu-devel] " Michael S. Tsirkin 2010-04-13 20:38 ` Eric Dumazet 2010-04-13 20:38 ` [Qemu-devel] " Eric Dumazet 2010-04-13 20:43 ` Michael S. Tsirkin 2010-04-13 20:43 ` [Qemu-devel] " Michael S. Tsirkin 2010-04-14 0:58 ` Herbert Xu 2010-04-14 0:58 ` [Qemu-devel] " Herbert Xu 2010-04-14 0:58 ` Herbert Xu 2010-04-14 11:55 ` David Miller 2010-04-14 11:55 ` [Qemu-devel] " David Miller 2010-04-14 11:55 ` David Miller 2015-02-01 11:20 ` David Woodhouse 2015-02-01 11:20 ` [Qemu-devel] " David Woodhouse 2015-02-01 12:26 ` Michael S. Tsirkin 2015-02-01 12:26 ` [Qemu-devel] " Michael S. Tsirkin 2015-02-01 13:33 ` David Woodhouse 2015-02-01 13:33 ` [Qemu-devel] " David Woodhouse 2015-02-01 20:19 ` David Miller 2015-02-01 20:19 ` [Qemu-devel] " David Miller 2015-02-01 21:29 ` David Woodhouse 2015-02-01 21:29 ` [Qemu-devel] " David Woodhouse 2015-02-02 5:07 ` David Miller 2015-02-02 5:07 ` [Qemu-devel] " David Miller 2015-02-02 5:07 ` David Miller 2015-02-02 7:27 ` David Woodhouse 2015-02-02 7:27 ` [Qemu-devel] " David Woodhouse 2015-02-02 8:24 ` Steffen Klassert 2015-02-02 8:24 ` [Qemu-devel] " Steffen Klassert 2015-02-02 15:30 ` David Woodhouse 2015-02-02 15:30 ` [Qemu-devel] " David Woodhouse 2015-02-02 15:23 ` Phil Sutter 2015-02-02 15:23 ` [Qemu-devel] " Phil Sutter 2015-02-02 15:47 ` David Woodhouse 2015-02-02 15:47 ` [Qemu-devel] " David Woodhouse 2015-02-04 0:19 ` David Miller 2015-02-04 0:19 ` [Qemu-devel] " David Miller 2015-02-04 6:35 ` David Woodhouse 2015-02-04 6:35 ` [Qemu-devel] " David Woodhouse 2021-06-25 15:56 ` Bringing the SSL VPN data path back in-kernel David Woodhouse 2010-04-21 11:35 ` [PATCH] tun: orphan an skb on tx Michael S. Tsirkin 2010-04-21 11:35 ` [Qemu-devel] " Michael S. Tsirkin 2010-04-21 11:46 ` Jan Kiszka 2010-04-21 11:46 ` [Qemu-devel] " Jan Kiszka 2010-04-21 11:45 ` Michael S. Tsirkin 2010-04-21 11:45 ` [Qemu-devel] " Michael S. Tsirkin 2010-04-21 19:16 ` [stable] " Greg KH 2010-04-21 19:16 ` [Qemu-devel] " Greg KH 2010-09-14 15:20 ` Michael S. Tsirkin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=4BC48F79.5090409@siemens.com \ --to=jan.kiszka@siemens.com \ --cc=David.Woodhouse@intel.com \ --cc=davem@davemloft.net \ --cc=herbert@gondor.hengli.com.au \ --cc=linux-kernel@vger.kernel.org \ --cc=mst@redhat.com \ --cc=netdev@vger.kernel.org \ --cc=paul.moore@hp.com \ --cc=qemu-devel@nongnu.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.