| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1397185075.6230.1.camel@localhost>
Date: Fri, 11 Apr 2014 10:57:55 +0800
From: Jason Wang <jasowang@...hat.com>
To: "Michael S. Tsirkin" <mst@...hat.com>
Cc: Steven Galgano <sgalgano@...acentlink.com>, davem@...emloft.net,
xemul@...allels.com, wuzhy@...ux.vnet.ibm.com, therbert@...gle.com,
yamato@...hat.com, richardcochran@...il.com,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Brian Adamson <Brian.Adamson@....navy.mil>,
Joseph Giovatto <jgiovatto@...acentlink.com>
Subject: Re: [PATCH] tuntap: add flow control to support back pressure
On Thu, 2014-04-10 at 13:29 +0300, Michael S. Tsirkin wrote:
> On Wed, Apr 09, 2014 at 10:19:40PM -0400, Steven Galgano wrote:
> > Add tuntap flow control support for use by back pressure routing protocols. Setting the new TUNSETIFF flag IFF_FLOW_CONTROL, will signal resources as unavailable when the tx queue limit is reached by issuing a netif_tx_stop_all_queues() rather than discarding frames. A netif_tx_wake_all_queues() is issued after reading a frame from the queue to signal resource availability.
> >
> > Back pressure capability was previously supported by the legacy tun default mode. This change restores that functionality, which was last present in v3.7.
> >
> > Reported-by: Brian Adamson <brian.adamson@....navy.mil>
> > Tested-by: Joseph Giovatto <jgiovatto@...acentlink.com>
> > Signed-off-by: Steven Galgano <sgalgano@...acentlink.com>
>
> I don't think it's a good idea.
>
> This trivial flow control really created more problems than it was worth.
>
> In particular this blocks all flows so it's trivially easy for one flow
> to block and starve all others: just send a bunch of packets to loopback
> destinations that get queued all over the place.
>
> Luckily it was never documented so we changed the default and nothing
> seems to break, but we won't be so lucky if we add an explicit API.
>
>
> One way to implement this would be with ubuf_info callback this is
> already invoked in most places where a packet might get stuck for a long
> time. It's still incomplete though: this will prevent head of queue
> blocking literally forever, but a single bad flow can still degrade
> performance significantly.
This is send queue for tuntap. Like all other real nics, we can solve
this through fairness qdiscs?
>
> Another alternative is to try and isolate the flows that we
> can handle and throttle them.
>
> It's all fixable but we really need to fix the issues *before*
> exposing the interface to userspace.
>
>
>
> > ---
> > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > index ee328ba..268130c 100644
> > --- a/drivers/net/tun.c
> > +++ b/drivers/net/tun.c
> > @@ -783,8 +783,19 @@ static netdev_tx_t tun_net_xmit(struct sk_buff *skb, struct net_device *dev)
> > * number of queues.
> > */
> > if (skb_queue_len(&tfile->socket.sk->sk_receive_queue) * numqueues
> > - >= dev->tx_queue_len)
> > - goto drop;
> > + >= dev->tx_queue_len) {
> > + if (tun->flags & TUN_FLOW_CONTROL) {
> > + /* Resources unavailable stop transmissions */
> > + netif_tx_stop_all_queues(dev);
> > +
> > + /* We won't see all dropped packets individually, so
> > + * over run error is more appropriate.
> > + */
> > + dev->stats.tx_fifo_errors++;
> > + } else {
> > + goto drop;
> > + }
> > + }
> >
> > if (unlikely(skb_orphan_frags(skb, GFP_ATOMIC)))
> > goto drop;
> > @@ -1362,6 +1373,9 @@ static ssize_t tun_do_read(struct tun_struct *tun, struct tun_file *tfile,
> > continue;
> > }
> >
> > + /* Wake in case resources previously signaled unavailable */
> > + netif_tx_wake_all_queues(tun->dev);
> > +
> > ret = tun_put_user(tun, tfile, skb, iv, len);
> > kfree_skb(skb);
> > break;
> > @@ -1550,6 +1564,9 @@ static int tun_flags(struct tun_struct *tun)
> > if (tun->flags & TUN_PERSIST)
> > flags |= IFF_PERSIST;
> >
> > + if (tun->flags & TUN_FLOW_CONTROL)
> > + flags |= IFF_FLOW_CONTROL;
> > +
> > return flags;
> > }
> >
> > @@ -1732,6 +1749,11 @@ static int tun_set_iff(struct net *net, struct file *file, struct ifreq *ifr)
> > else
> > tun->flags &= ~TUN_TAP_MQ;
> >
> > + if (ifr->ifr_flags & IFF_FLOW_CONTROL)
> > + tun->flags |= TUN_FLOW_CONTROL;
> > + else
> > + tun->flags &= ~TUN_FLOW_CONTROL;
> > +
> > /* Make sure persistent devices do not get stuck in
> > * xoff state.
> > */
> > @@ -1900,7 +1922,8 @@ static long __tun_chr_ioctl(struct file *file, unsigned int cmd,
> > * This is needed because we never checked for invalid flags on
> > * TUNSETIFF. */
> > return put_user(IFF_TUN | IFF_TAP | IFF_NO_PI | IFF_ONE_QUEUE |
> > - IFF_VNET_HDR | IFF_MULTI_QUEUE,
> > + IFF_VNET_HDR | IFF_MULTI_QUEUE |
> > + IFF_FLOW_CONTROL,
> > (unsigned int __user*)argp);
> > } else if (cmd == TUNSETQUEUE)
> > return tun_set_queue(file, &ifr);
> > diff --git a/include/uapi/linux/if_tun.h b/include/uapi/linux/if_tun.h
> > index e9502dd..bcf2790 100644
> > --- a/include/uapi/linux/if_tun.h
> > +++ b/include/uapi/linux/if_tun.h
> > @@ -36,6 +36,7 @@
> > #define TUN_PERSIST 0x0100
> > #define TUN_VNET_HDR 0x0200
> > #define TUN_TAP_MQ 0x0400
> > +#define TUN_FLOW_CONTROL 0x0800
> >
> > /* Ioctl defines */
> > #define TUNSETNOCSUM _IOW('T', 200, int)
> > @@ -70,6 +71,7 @@
> > #define IFF_MULTI_QUEUE 0x0100
> > #define IFF_ATTACH_QUEUE 0x0200
> > #define IFF_DETACH_QUEUE 0x0400
> > +#define IFF_FLOW_CONTROL 0x0010
> > /* read-only flag */
> > #define IFF_PERSIST 0x0800
> > #define IFF_NOFILTER 0x1000
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists