* [PATCH] skbuff: struct ubuf_info callback type safety
From: Michael S. Tsirkin @ 2012-04-09 10:24 UTC (permalink / raw)
To: linux-kernel, kvm, virtualization, netdev
Cc: Shirley Ma, Stephen Hemminger, David S. Miller, Eric Dumazet,
Ian Campbell
The skb struct ubuf_info callback gets passed struct ubuf_info
itself, not the arg value as the field name and the function signature
seem to imply. Rename the arg field to ctx to match usage,
add documentation and change the callback argument type
to make usage clear and to have compiler check correctness.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
drivers/vhost/net.c | 2 +-
drivers/vhost/vhost.c | 5 ++---
drivers/vhost/vhost.h | 2 +-
include/linux/skbuff.h | 7 ++++---
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index f0da2c3..1f21d2a 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -238,7 +238,7 @@ static void handle_tx(struct vhost_net *net)
vq->heads[vq->upend_idx].len = len;
ubuf->callback = vhost_zerocopy_callback;
- ubuf->arg = vq->ubufs;
+ ubuf->ctx = vq->ubufs;
ubuf->desc = vq->upend_idx;
msg.msg_control = ubuf;
msg.msg_controllen = sizeof(ubuf);
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 947f00d..51e4c1e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -1598,10 +1598,9 @@ void vhost_ubuf_put_and_wait(struct vhost_ubuf_ref *ubufs)
kfree(ubufs);
}
-void vhost_zerocopy_callback(void *arg)
+void vhost_zerocopy_callback(struct ubuf_info *ubuf)
{
- struct ubuf_info *ubuf = arg;
- struct vhost_ubuf_ref *ubufs = ubuf->arg;
+ struct vhost_ubuf_ref *ubufs = ubuf->ctx;
struct vhost_virtqueue *vq = ubufs->vq;
/* set len = 1 to mark this desc buffers done DMA */
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 8dcf4cc..8de1fd5 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -188,7 +188,7 @@ bool vhost_enable_notify(struct vhost_dev *, struct vhost_virtqueue *);
int vhost_log_write(struct vhost_virtqueue *vq, struct vhost_log *log,
unsigned int log_num, u64 len);
-void vhost_zerocopy_callback(void *arg);
+void vhost_zerocopy_callback(struct ubuf_info *);
int vhost_zerocopy_signal_used(struct vhost_virtqueue *vq);
#define vq_err(vq, fmt, ...) do { \
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 3337027..4c3f138 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -238,11 +238,12 @@ enum {
/*
* The callback notifies userspace to release buffers when skb DMA is done in
* lower device, the skb last reference should be 0 when calling this.
- * The desc is used to track userspace buffer index.
+ * The ctx field is used to track device context.
+ * The desc field is used to track userspace buffer index.
*/
struct ubuf_info {
- void (*callback)(void *);
- void *arg;
+ void (*callback)(struct ubuf_info *);
+ void *ctx;
unsigned long desc;
};
--
1.7.9.111.gf3fb0
^ permalink raw reply related
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Eric Dumazet @ 2012-04-09 9:13 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Herbert Xu, netdev, linux-kernel, David S. Miller,
Jamal Hadi Salim, Stephen Hemminger, Jason Wang, Neil Horman,
Jiri Pirko, Jeff Kirsher, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409084208.GA28362@redhat.com>
On Mon, 2012-04-09 at 11:42 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 09, 2012 at 04:39:58PM +0800, Herbert Xu wrote:
> > On Mon, Apr 09, 2012 at 11:34:02AM +0300, Michael S. Tsirkin wrote:
> > >
> > > Puzzled. This patch orphans skbs only if the destination device sets
> > > IFF_TX_CAN_STALL. Since the physical NIC doesn't stall forever it never
> > > sets this flag.
> > >
> > > So it seems that this patch should not affect the configuration
> > > you describe at all.
> > >
> > > Could you please clarify? What did I miss?
> >
> > Oops, you're right. Somehow I was thinking you were patching
> > virtio-net instead of tun.
> >
> > Cheers,
>
> Cool. Want to ack the patch then just to make it clear you are happy?
I am not happy with this patch.
Since its Easter, I dont have much time to comment right now, I'll do
that tomorrow.
^ permalink raw reply
* [PATCH] net/wireless/wext-core.c: add missing kfree
From: Julia Lawall @ 2012-04-09 9:01 UTC (permalink / raw)
To: John W. Linville
Cc: kernel-janitors-u79uwXL29TY76Z2rM5mHXA, David S. Miller,
linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
Free extra as done in the error-handling code just above.
Signed-off-by: Julia Lawall <Julia.Lawall-L2FTfq7BK8M@public.gmane.org>
---
net/wireless/wext-core.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 9f544c9..22adfeb 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -781,8 +781,10 @@ static int ioctl_standard_iw_point(struct iw_point *iwp, unsigned int cmd,
if (cmd == SIOCSIWENCODEEXT) {
struct iw_encode_ext *ee = (void *) extra;
- if (iwp->length < sizeof(*ee) + ee->key_len)
- return -EFAULT;
+ if (iwp->length < sizeof(*ee) + ee->key_len) {
+ err = -EFAULT;
+ goto out;
+ }
}
}
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [RFC] API to modify /proc/sys/net/ipv4/ip_local_reserved_ports
From: Cong Wang @ 2012-04-09 8:43 UTC (permalink / raw)
To: Helge Deller
Cc: Octavian Purdila, netdev, David Miller, Andrew Morton,
Eric W. Biederman, Frank Danapfel, Laszlo Ersek, shemminger
In-Reply-To: <4F7CADE8.3060205@gmx.de>
On Wed, 2012-04-04 at 22:24 +0200, Helge Deller wrote:
> I would like to follow up on my last patch series to be able to modify
> the contents of the /proc/sys/net/ipv4/ip_local_reserved_ports port list
> from userspace.
>
> My last patch (https://lkml.org/lkml/2012/3/10/187) was based on
> modifications to the proc interface, which - based on the feedback here
> on the list - seemed to not be the right way to go (although I personally
> still like the idea very much :-)).
>
> Anyway, with this RFC I would like to get feedback about a new proposed
> API and attached kernel patch.
>
> The idea is to introduce a new <optname> value for get/setsockopt()
> named SO_RESERVED_PORTS to get/set the ip_local_reserved_ports
> bitmap via standard get/setsockopt() syscalls.
> As far as I understand this seems to be similiar to how iptables works.
>
> An untested kernel patch for review and feedback is attached below.
>
> In userspace it then would be possible to write a new tool or to extend
> for example the "ip" tool to accept commands like:
> $> ip reserved_ports add 100-2000
> $> ip reserved_ports remove 50-60
> $> ip reserved_ports list (to show current reserved port list)
>
> This userspace tool could then read the port bitmap from kernel via
> a) socket(PF_INET, SOCK_RAW, IPPROTO_RAW)
> b) getsockopt(3, SOL_SOCKET, SO_RESERVED_PORTS, <bitmaplist>)
> and write back the results after modification via
> c) setsockopt(3, SOL_SOCKET, SO_RESERVED_PORTS, <bitmaplist>)
>
> Would that be an acceptable solution?
Hmm, it is indeed that bitmap fits for syscall rather than /proc file.
But it seems that using getsockopt()/setsockopt() makes it like it is a
per-socket setting, actually it is a system-wide setting. So I am
wondering if exporting a binary /proc file for this is a better
solution.
Thanks.
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-09 8:42 UTC (permalink / raw)
To: Herbert Xu
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409083958.GA3951@gondor.apana.org.au>
On Mon, Apr 09, 2012 at 04:39:58PM +0800, Herbert Xu wrote:
> On Mon, Apr 09, 2012 at 11:34:02AM +0300, Michael S. Tsirkin wrote:
> >
> > Puzzled. This patch orphans skbs only if the destination device sets
> > IFF_TX_CAN_STALL. Since the physical NIC doesn't stall forever it never
> > sets this flag.
> >
> > So it seems that this patch should not affect the configuration
> > you describe at all.
> >
> > Could you please clarify? What did I miss?
>
> Oops, you're right. Somehow I was thinking you were patching
> virtio-net instead of tun.
>
> Cheers,
Cool. Want to ack the patch then just to make it clear you are happy?
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH] net/key/af_key.c: add missing kfree_skb
From: Julia Lawall @ 2012-04-09 8:41 UTC (permalink / raw)
To: David S. Miller; +Cc: kernel-janitors, netdev, linux-kernel
From: Julia Lawall <Julia.Lawall@lip6.fr>
At the point of this error-handling code, alloc_skb has succeded, so free
the resulting skb by jumping to the err label.
Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
---
net/key/af_key.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/key/af_key.c b/net/key/af_key.c
index 11dbb22..7e5d927 100644
--- a/net/key/af_key.c
+++ b/net/key/af_key.c
@@ -3480,7 +3480,7 @@ static int pfkey_send_migrate(const struct xfrm_selector *sel, u8 dir, u8 type,
/* Addresses to be used by KM for negotiation, if ext is available */
if (k != NULL && (set_sadb_kmaddress(skb, k) < 0))
- return -EINVAL;
+ goto err;
/* selector src */
set_sadb_address(skb, sasize_sel, SADB_EXT_ADDRESS_SRC, sel);
^ permalink raw reply related
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Herbert Xu @ 2012-04-09 8:39 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409083402.GA28249@redhat.com>
On Mon, Apr 09, 2012 at 11:34:02AM +0300, Michael S. Tsirkin wrote:
>
> Puzzled. This patch orphans skbs only if the destination device sets
> IFF_TX_CAN_STALL. Since the physical NIC doesn't stall forever it never
> sets this flag.
>
> So it seems that this patch should not affect the configuration
> you describe at all.
>
> Could you please clarify? What did I miss?
Oops, you're right. Somehow I was thinking you were patching
virtio-net instead of tun.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-09 8:34 UTC (permalink / raw)
To: Herbert Xu
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409082919.GA3471@gondor.apana.org.au>
On Mon, Apr 09, 2012 at 04:29:20PM +0800, Herbert Xu wrote:
> On Mon, Apr 09, 2012 at 10:39:54AM +0300, Michael S. Tsirkin wrote:
> > On Mon, Apr 09, 2012 at 03:33:54PM +0800, Herbert Xu wrote:
> > > On Mon, Apr 09, 2012 at 10:28:49AM +0300, Michael S. Tsirkin wrote:
> > > >
> > > > > 1) Doesn't this break local UDP push-back?
> > > >
> > > > What is meant by UDP pushback here? Two tap
> > > > devices communicating by UDP packets locally?
> > > > This was always broken, see below.
> > >
> > > I mean push-back from UDP transmission to the physical NIC.
> > >
> > > Your patch breaks that
> >
> > I think there's some misunderstanding. pushback is only disabled
> > for destinations that set IFF_TX_CAN_STALL. I expect that
> > no physical NICs set this flag - only tun and possibly
> > other userspace-controlled devices in the future.
>
> I'm talking about an app running in the guest transmitting UDP
> to the physical NIC via virtio/vhost. This will break with your
> patch.
>
> Cheers,
Puzzled. This patch orphans skbs only if the destination device sets
IFF_TX_CAN_STALL. Since the physical NIC doesn't stall forever it never
sets this flag.
So it seems that this patch should not affect the configuration
you describe at all.
Could you please clarify? What did I miss?
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Herbert Xu @ 2012-04-09 8:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409073953.GD12014@redhat.com>
On Mon, Apr 09, 2012 at 10:39:54AM +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 09, 2012 at 03:33:54PM +0800, Herbert Xu wrote:
> > On Mon, Apr 09, 2012 at 10:28:49AM +0300, Michael S. Tsirkin wrote:
> > >
> > > > 1) Doesn't this break local UDP push-back?
> > >
> > > What is meant by UDP pushback here? Two tap
> > > devices communicating by UDP packets locally?
> > > This was always broken, see below.
> >
> > I mean push-back from UDP transmission to the physical NIC.
> >
> > Your patch breaks that
>
> I think there's some misunderstanding. pushback is only disabled
> for destinations that set IFF_TX_CAN_STALL. I expect that
> no physical NICs set this flag - only tun and possibly
> other userspace-controlled devices in the future.
I'm talking about an app running in the guest transmitting UDP
to the physical NIC via virtio/vhost. This will break with your
patch.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-09 7:39 UTC (permalink / raw)
To: Herbert Xu
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409073354.GA3218@gondor.apana.org.au>
On Mon, Apr 09, 2012 at 03:33:54PM +0800, Herbert Xu wrote:
> On Mon, Apr 09, 2012 at 10:28:49AM +0300, Michael S. Tsirkin wrote:
> >
> > > 1) Doesn't this break local UDP push-back?
> >
> > What is meant by UDP pushback here? Two tap
> > devices communicating by UDP packets locally?
> > This was always broken, see below.
>
> I mean push-back from UDP transmission to the physical NIC.
>
> Your patch breaks that
I think there's some misunderstanding. pushback is only disabled
for destinations that set IFF_TX_CAN_STALL. I expect that
no physical NICs set this flag - only tun and possibly
other userspace-controlled devices in the future.
> as now the guest will have no push-back
> whatsoever so anything that transmits UDP without protocol-level
> congestion control will start dropping most of their packets.
>
> Granted you can argue that these apps are broken, but they do
> exist and we've always catered for them, both on baremetal and
> under virtualisation.
> > Thus we get this situation
> > tap1 sends packets, some of them to tap2, tap2 does not consume them,
> > as a result tap2 queue overflows, tap2 stops forever and
> > packets get queued in the qdisc, now tap1
> > send buffer gets full so it can not communicate to any destination.
> >
> > So the problem is one VM can block all networking from another one.
>
> This should be addressed in the backend, as it can distinguish
> between packets going out to physical and packets stuck going to
> a local VM.
Sorry I still have no clue what you call a backend.
Could you clarify please? All packets from a tun device
go to userspace.
> In the latter case you can then duplicate and release
> the sender's memory.
>
> Cheers,
The problem this patch is trying to address is tun device
can get stopped forever, which causes packets to
accumulate in qdisc again forever. So tun does not
get a chance to release sender's memory for these packets.
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Herbert Xu @ 2012-04-09 7:33 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120409072849.GA12014@redhat.com>
On Mon, Apr 09, 2012 at 10:28:49AM +0300, Michael S. Tsirkin wrote:
>
> > 1) Doesn't this break local UDP push-back?
>
> What is meant by UDP pushback here? Two tap
> devices communicating by UDP packets locally?
> This was always broken, see below.
I mean push-back from UDP transmission to the physical NIC.
Your patch breaks that as now the guest will have no push-back
whatsoever so anything that transmits UDP without protocol-level
congestion control will start dropping most of their packets.
Granted you can argue that these apps are broken, but they do
exist and we've always catered for them, both on baremetal and
under virtualisation.
> Thus we get this situation
> tap1 sends packets, some of them to tap2, tap2 does not consume them,
> as a result tap2 queue overflows, tap2 stops forever and
> packets get queued in the qdisc, now tap1
> send buffer gets full so it can not communicate to any destination.
>
> So the problem is one VM can block all networking from another one.
This should be addressed in the backend, as it can distinguish
between packets going out to physical and packets stuck going to
a local VM. In the latter case you can then duplicate and release
the sender's memory.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Michael S. Tsirkin @ 2012-04-09 7:28 UTC (permalink / raw)
To: Herbert Xu
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120408234951.GA15993@gondor.apana.org.au>
On Mon, Apr 09, 2012 at 07:49:51AM +0800, Herbert Xu wrote:
> On Sun, Apr 08, 2012 at 08:13:25PM +0300, Michael S. Tsirkin wrote:
> > commit 0110d6f22f392f976e84ab49da1b42f85b64a3c5
> > tun: orphan an skb on tx
> > Fixed a configuration where skbs get queued
> > at the tun device forever, blocking senders.
> >
> > However this fix isn't waterproof:
> > userspace can control whether the interface
> > is stopped, and if it is, packets
> > get queued in the qdisc, again potentially forever.
> >
> > Complete the fix by setting a private flag and orphaning
> > at the qdisc level.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> 1) Doesn't this break local UDP push-back?
What is meant by UDP pushback here? Two tap
devices communicating by UDP packets locally?
This was always broken, see below.
> 2) Isn't the stall a bug in the backend and isn't this just
> papering over that?
>
> Cheers,
What do you mean by the backend? userspace? Yes, the stall is a result of
userspace not consuming packets:
if (skb_queue_len(&tun->socket.sk->sk_receive_queue) >= dev->tx_queue_len) {
if (!(tun->flags & TUN_ONE_QUEUE)) {
/* Normal queueing mode. */
/* Packet scheduler handles dropping of further packets. */
netif_stop_queue(dev);
/* We won't see all dropped packets individually, so overrun
* error is more appropriate. */
dev->stats.tx_fifo_errors++;
Thus we get this situation
tap1 sends packets, some of them to tap2, tap2 does not consume them,
as a result tap2 queue overflows, tap2 stops forever and
packets get queued in the qdisc, now tap1
send buffer gets full so it can not communicate to any destination.
So the problem is one VM can block all networking from another one.
As a solution this patch is always changing ownership if we're going
into a hostile device: it just does this early in qdisc instead of
at xmit time.
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH 2/2] qlcnic: Remove redundant NULL check on kfree().
From: santosh nayak @ 2012-04-09 6:11 UTC (permalink / raw)
To: anirban.chakraborty, rajesh.borundia
Cc: sony.chacko, linux-driver, netdev, kernel-janitors, Santosh Nayak
From: Santosh Nayak <santoshprasadnayak@gmail.com>
kfree() checks for NULL before freeing the memory.
Remove redundant NULL check.
This is just a clean up and also looks good from performance point of view.
Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 14 ++++----------
1 files changed, 4 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index d30b9b8..75b202d 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -160,9 +160,7 @@ qlcnic_alloc_sds_rings(struct qlcnic_recv_context *recv_ctx, int count)
static void
qlcnic_free_sds_rings(struct qlcnic_recv_context *recv_ctx)
{
- if (recv_ctx->sds_rings != NULL)
- kfree(recv_ctx->sds_rings);
-
+ kfree(recv_ctx->sds_rings);
recv_ctx->sds_rings = NULL;
}
@@ -1714,10 +1712,8 @@ static void __devexit qlcnic_remove(struct pci_dev *pdev)
qlcnic_detach(adapter);
- if (adapter->npars != NULL)
- kfree(adapter->npars);
- if (adapter->eswitch != NULL)
- kfree(adapter->eswitch);
+ kfree(adapter->npars);
+ kfree(adapter->eswitch);
qlcnic_clr_all_drv_state(adapter, 0);
@@ -1887,9 +1883,7 @@ void qlcnic_alloc_lb_filters_mem(struct qlcnic_adapter *adapter)
static void qlcnic_free_lb_filters_mem(struct qlcnic_adapter *adapter)
{
- if (adapter->fhash.fmax && adapter->fhash.fhead)
- kfree(adapter->fhash.fhead);
-
+ kfree(adapter->fhash.fhead);
adapter->fhash.fhead = NULL;
adapter->fhash.fmax = 0;
}
--
1.7.4.4
^ permalink raw reply related
* [PATCH 1/2] qlcnic: Add default swtich case in 'qlcnic_can_start_firmware()'
From: santosh nayak @ 2012-04-09 5:59 UTC (permalink / raw)
To: anirban.chakraborty, rajesh.borundia
Cc: sony.chacko, linux-driver, netdev, kernel-janitors, Santosh Nayak
From: Santosh Nayak <santoshprasadnayak@gmail.com>
QLCRD32() may return bad state.
Its good to have default switch case:
1. to avoid unexpected system behaviour.
2. to avoid extra computation thats happening
after switch case.
Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
index 75c32e8..d30b9b8 100644
--- a/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
+++ b/drivers/net/ethernet/qlogic/qlcnic/qlcnic_main.c
@@ -2787,6 +2787,11 @@ qlcnic_can_start_firmware(struct qlcnic_adapter *adapter)
case QLCNIC_DEV_INITIALIZING:
case QLCNIC_DEV_QUISCENT:
break;
+
+ default:
+ dev_err(&adapter->pdev->dev, "Invalid state\n");
+ qlcnic_api_unlock(adapter);
+ return -EINVAL;
}
qlcnic_api_unlock(adapter);
--
1.7.4.4
^ permalink raw reply related
* Re: [PATCH v17 00/15] seccomp_filter: BPF-based syscall filtering
From: James Morris @ 2012-04-09 3:48 UTC (permalink / raw)
To: Andrew Morton
Cc: Will Drewry, linux-kernel, linux-security-module, linux-arch,
linux-doc, kernel-hardening, netdev, x86, arnd, davem, hpa, mingo,
Oleg Nesterov, peterz, rdunlap, mcgrathr, tglx, luto, eparis,
serge.hallyn, djm, scarybeasts, indan, pmoore, corbet,
eric.dumazet, markus, coreyb, keescook, Oleg Nesterov
In-Reply-To: <20120406142846.fd8dea70.akpm@linux-foundation.org>
On Fri, 6 Apr 2012, Andrew Morton wrote:
> > I've offered to take this via my tree -- if anyone else wants to merge it
> > via theirs instead, holler.
>
> OK by me, especially if it means that you'll carefully (re)review it! I
> had some minor quibbles with v17.
Sure.
>
> It would be good if Oleg could take a look at the signal changes, if he
> has not yet done so?
>
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* Re: [BUGFIX][PATCH 3/3] memcg/tcp: ignore tcp usage before accounting started
From: Glauber Costa @ 2012-04-09 1:44 UTC (permalink / raw)
To: KAMEZAWA Hiroyuki; +Cc: David Miller, netdev, akpm
In-Reply-To: <4F823443.9070908@jp.fujitsu.com>
On 04/08/2012 09:58 PM, KAMEZAWA Hiroyuki wrote:
> (2012/04/04 7:31), Glauber Costa wrote:
>
>> On 04/02/2012 07:41 AM, David Miller wrote:
>>> From: Glauber Costa<glommer@parallels.com>
>>> Date: Thu, 29 Mar 2012 11:21:07 +0200
>>>
>>>> On 03/29/2012 09:10 AM, KAMEZAWA Hiroyuki wrote:
>>>>> tcp memcontrol starts accouting after res->limit is set. So, if a sockets
>>>>> starts before setting res->limit, there are already used resource.
>>>>> After setting res->limit, the resource (already used) will be uncharged and
>>>>> make res_counter below 0 because they are not charged. This causes warning.
>>>>>
>>>>> This patch fixes that by adding res_counter_uncharge_nowarn().
>>>>> (*) We cannot avoid this while we have 'account start' switch.
>>>>>
>>>>> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>>>
>>>> Fine by me.
>>>>
>>>> Acked-by: Glauber Costa<glommer@parallels.com>
>>>
>>> I'm not applying patches that simply ignore accounting counter
>>> underflows.
>>>
>>> You must either:
>>>
>>> 1) Integrate the socket's existing usage when the limit is set.
>>>
>>> 2) Avoid accounting completely for a socket that started before
>>> the limit was set.
>>>
>>> No half-way solutions, please. Otherwise it is impossible to design
>>> validations of the resource usage for a particular socket or group of
>>> sockets, because they can always be potentially "wrong" and over the
>>> limit. That's a design for a buggy system.
>>>
>>>
>> Kame,
>>
>> I agree with Dave FWIW.
>>
>> We should be able to do this by dropping the reference count when the
>> cgroup is finally destroyed, instead of from the remove callback. At
>> that point, no more pending sockets should be attached to it.
>>
>> Prior to increasing the static key, they are all assigned to the global
>> cgroup, so we shouldn't care about them.
>>
>
> Could you do the fix ?
>
> Thanks,
> -Kame
>
>
I sent you a version already. Please just make sure it works for you
^ permalink raw reply
* Re: [BUGFIX][PATCH 3/3] memcg/tcp: ignore tcp usage before accounting started
From: KAMEZAWA Hiroyuki @ 2012-04-09 0:58 UTC (permalink / raw)
To: Glauber Costa; +Cc: David Miller, netdev, akpm
In-Reply-To: <4F7B7A54.9000501@parallels.com>
(2012/04/04 7:31), Glauber Costa wrote:
> On 04/02/2012 07:41 AM, David Miller wrote:
>> From: Glauber Costa<glommer@parallels.com>
>> Date: Thu, 29 Mar 2012 11:21:07 +0200
>>
>>> On 03/29/2012 09:10 AM, KAMEZAWA Hiroyuki wrote:
>>>> tcp memcontrol starts accouting after res->limit is set. So, if a sockets
>>>> starts before setting res->limit, there are already used resource.
>>>> After setting res->limit, the resource (already used) will be uncharged and
>>>> make res_counter below 0 because they are not charged. This causes warning.
>>>>
>>>> This patch fixes that by adding res_counter_uncharge_nowarn().
>>>> (*) We cannot avoid this while we have 'account start' switch.
>>>>
>>>> Signed-off-by: KAMEZAWA Hiroyuki<kamezawa.hiroyu@jp.fujitsu.com>
>>>
>>> Fine by me.
>>>
>>> Acked-by: Glauber Costa<glommer@parallels.com>
>>
>> I'm not applying patches that simply ignore accounting counter
>> underflows.
>>
>> You must either:
>>
>> 1) Integrate the socket's existing usage when the limit is set.
>>
>> 2) Avoid accounting completely for a socket that started before
>> the limit was set.
>>
>> No half-way solutions, please. Otherwise it is impossible to design
>> validations of the resource usage for a particular socket or group of
>> sockets, because they can always be potentially "wrong" and over the
>> limit. That's a design for a buggy system.
>>
>>
> Kame,
>
> I agree with Dave FWIW.
>
> We should be able to do this by dropping the reference count when the
> cgroup is finally destroyed, instead of from the remove callback. At
> that point, no more pending sockets should be attached to it.
>
> Prior to increasing the static key, they are all assigned to the global
> cgroup, so we shouldn't care about them.
>
Could you do the fix ?
Thanks,
-Kame
^ permalink raw reply
* Re: [PATCH] net: orphan queued skbs if device tx can stall
From: Herbert Xu @ 2012-04-08 23:49 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: netdev, linux-kernel, David S. Miller, Jamal Hadi Salim,
Stephen Hemminger, Jason Wang, Neil Horman, Jiri Pirko,
Jeff Kirsher, Eric Dumazet, Michał Mirosław,
Ben Hutchings
In-Reply-To: <20120408171323.GA16012@redhat.com>
On Sun, Apr 08, 2012 at 08:13:25PM +0300, Michael S. Tsirkin wrote:
> commit 0110d6f22f392f976e84ab49da1b42f85b64a3c5
> tun: orphan an skb on tx
> Fixed a configuration where skbs get queued
> at the tun device forever, blocking senders.
>
> However this fix isn't waterproof:
> userspace can control whether the interface
> is stopped, and if it is, packets
> get queued in the qdisc, again potentially forever.
>
> Complete the fix by setting a private flag and orphaning
> at the qdisc level.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
1) Doesn't this break local UDP push-back?
2) Isn't the stall a bug in the backend and isn't this just
papering over that?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* [PATCH iproute2] tc: man: choke counts packets, not bytes
From: Florian Westphal @ 2012-04-08 22:34 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
---
man/man8/tc-choke.8 | 11 ++++++-----
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/man/man8/tc-choke.8 b/man/man8/tc-choke.8
index 7b213be..620c7f6 100644
--- a/man/man8/tc-choke.8
+++ b/man/man8/tc-choke.8
@@ -4,11 +4,11 @@ choke \- choose and keep scheduler
.SH SYNOPSIS
.B tc qdisc ... choke
.B limit
-bytes
+packets
.B min
-bytes
+packets
.B max
-bytes
+packets
.B avpkt
bytes
.B burst
@@ -22,7 +22,7 @@ chance
CHOKe (CHOose and Keep for responsive flows, CHOose and Kill for unresponsive flows)
is a classless qdisc designed to both identify and penalize flows that monopolize the
-queue. CHOKe is a variation of RED, and the configuration is the same as RED.
+queue. CHOKe is a variation of RED, and the configuration is similar to RED.
.SH ALGORITHM
Once the queue hits a certain average length, a random packet is drawn from the
@@ -39,7 +39,8 @@ If the queue length exceeds
The marking probability computation is the same as used by the RED qdisc.
.SH PARAMETERS
-The parameters are the same as for RED, see
+The parameters are the same as for RED, except that RED uses bytes whereas choke
+counts packets. See
.BR tc-red (8)
for a description.
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH 5/9] ipvs: use adaptive pause in master thread
From: Julian Anastasov @ 2012-04-08 20:12 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: Simon Horman, lvs-devel, netdev, netfilter-devel, Wensong Zhang
In-Reply-To: <20120405150553.GA8144@1984>
Hello,
On Thu, 5 Apr 2012, Pablo Neira Ayuso wrote:
> I think you can control when the kernel thread is woken up with a
> counting semaphore. The counter of that semaphore will be initially
> set to zero. Then, you can up() the semaphore once per new buffer
> that you enqueue to the sender.
>
> feeder:
> add message to sync buffer
> if buffer full:
> enqueue buffer to sender_thread
> up(s)
>
> sender_thread:
> while (1) {
> down(s)
> retrieve message from queue
> send message
> }
>
> It seems to me like the classical producer/consumer problem that you
> can resolve with semaphores.
May be it is possible to use up/down but we
have to handle the kthread_should_stop check and also
I prefer to reduce the wakeup events. So, I'm trying
another solution which is appended just for review.
> Under congestion the situation is complicated. At some point you'll
> end up dropping messages.
>
> You may want to increase the socket queue to delay the moment at which
> we start dropping messages. You can expose the socke buffer length via
> /proc interface I guess (not sure if you're already doing that or
> suggesting to use the global socket buffer length).
I'm still thinking if sndbuf value should be exported,
currently users have to modify the global default/max value.
But in below version I'm trying to handle the sndbuf overflow
by blocking for write_space event. By this way we should work
with any sndbuf configuration.
> You also can define some mechanism to reduce the amount of events,
> some state filtering so you only propagate important states.
>
> Some partially reliable protocol, so the backup can request messages
> that got lost in a smart way would can also in handy. Basically, the
> master only retransmits the current state, not the whole sequence of
> messages (this is good under congestion, since you save messages).
> I implement that in conntrackd, but that's more complex solution,
> of course. I'd start with something simple.
The patch "reduce sync rate with time thresholds"
that follows the discussed one in the changeset has such
purpose to reduce the events, in tests the sync traffic is
reduced ~10 times. But it does not modify the current
protocol, it adds a very limited logic for retransmissions.
Here is the new version for review. It compiles
and does not crash in simple tests.
ipvs: wakeup master thread
High rate of sync messages in master can lead to
overflowing the socket buffer and dropping the messages.
Fixed pause of 1 second is not suitable for loaded masters,
so allow packet processing to wakeup the master thread
once per 32 messages (IPVS_SYNC_WAKEUP_RATE), so that
we do not send in long bursts even when socket buffer is
very large.
Add hard limit for the queued messages before sending
by using "sync_qlen_max" sysctl var. It defaults to 1/32 of
the memory pages but actually represents number of messages.
It will protect us from allocating large parts of memory
when the sending rate is lower than the queuing rate.
Change the master thread to detect and block on
SNDBUF overflow, so that we do not drop messages when
the socket limit is low but the sync_qlen_max limit is
not reached. On ENOBUFS or other errors just drop the
messages.
Use lower pause (200ms) to wait for messages, it
takes effect when the sync rate is low. It has two purposes,
to limit the delay for queued messages and to avoid
many wakeups when no messages are queued.
Finally, make sure kthread_should_stop is checked
properly in TASK_INTERRUPTIBLE state when going to sleep
to avoid delays in thread stopping.
Signed-off-by: Julian Anastasov <ja@ssi.bg>
---
include/net/ip_vs.h | 15 ++++++++
net/netfilter/ipvs/ip_vs_ctl.c | 8 ++++
net/netfilter/ipvs/ip_vs_sync.c | 74 +++++++++++++++++++++++++++++---------
3 files changed, 79 insertions(+), 18 deletions(-)
diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 2bdee51..539f557 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -870,6 +870,7 @@ struct netns_ipvs {
#endif
int sysctl_snat_reroute;
int sysctl_sync_ver;
+ int sysctl_sync_qlen_max;
int sysctl_cache_bypass;
int sysctl_expire_nodest_conn;
int sysctl_expire_quiescent_template;
@@ -890,6 +891,8 @@ struct netns_ipvs {
struct timer_list est_timer; /* Estimation timer */
/* ip_vs_sync */
struct list_head sync_queue;
+ int sync_queue_len;
+ unsigned int sync_queue_sent;
spinlock_t sync_lock;
struct ip_vs_sync_buff *sync_buff;
spinlock_t sync_buff_lock;
@@ -912,6 +915,8 @@ struct netns_ipvs {
#define DEFAULT_SYNC_THRESHOLD 3
#define DEFAULT_SYNC_PERIOD 50
#define DEFAULT_SYNC_VER 1
+#define IPVS_SYNC_WAKEUP_RATE 32
+#define IPVS_SYNC_QLEN_MAX (IPVS_SYNC_WAKEUP_RATE * 4)
#ifdef CONFIG_SYSCTL
@@ -930,6 +935,11 @@ static inline int sysctl_sync_ver(struct netns_ipvs *ipvs)
return ipvs->sysctl_sync_ver;
}
+static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+{
+ return ipvs->sysctl_sync_qlen_max;
+}
+
#else
static inline int sysctl_sync_threshold(struct netns_ipvs *ipvs)
@@ -947,6 +957,11 @@ static inline int sysctl_sync_ver(struct netns_ipvs *ipvs)
return DEFAULT_SYNC_VER;
}
+static inline int sysctl_sync_qlen_max(struct netns_ipvs *ipvs)
+{
+ return IPVS_SYNC_QLEN_MAX;
+}
+
#endif
/*
diff --git a/net/netfilter/ipvs/ip_vs_ctl.c b/net/netfilter/ipvs/ip_vs_ctl.c
index 964d426..e3280ae 100644
--- a/net/netfilter/ipvs/ip_vs_ctl.c
+++ b/net/netfilter/ipvs/ip_vs_ctl.c
@@ -1718,6 +1718,12 @@ static struct ctl_table vs_vars[] = {
.proc_handler = &proc_do_sync_mode,
},
{
+ .procname = "sync_qlen_max",
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec,
+ },
+ {
.procname = "cache_bypass",
.maxlen = sizeof(int),
.mode = 0644,
@@ -3662,6 +3668,8 @@ int __net_init ip_vs_control_net_init_sysctl(struct net *net)
tbl[idx++].data = &ipvs->sysctl_snat_reroute;
ipvs->sysctl_sync_ver = 1;
tbl[idx++].data = &ipvs->sysctl_sync_ver;
+ ipvs->sysctl_sync_qlen_max = nr_free_buffer_pages() / 32;
+ tbl[idx++].data = &ipvs->sysctl_sync_qlen_max;
tbl[idx++].data = &ipvs->sysctl_cache_bypass;
tbl[idx++].data = &ipvs->sysctl_expire_nodest_conn;
tbl[idx++].data = &ipvs->sysctl_expire_quiescent_template;
diff --git a/net/netfilter/ipvs/ip_vs_sync.c b/net/netfilter/ipvs/ip_vs_sync.c
index 0e36679..b36f89b 100644
--- a/net/netfilter/ipvs/ip_vs_sync.c
+++ b/net/netfilter/ipvs/ip_vs_sync.c
@@ -312,6 +312,7 @@ static inline struct ip_vs_sync_buff *sb_dequeue(struct netns_ipvs *ipvs)
struct ip_vs_sync_buff,
list);
list_del(&sb->list);
+ ipvs->sync_queue_len--;
}
spin_unlock_bh(&ipvs->sync_lock);
@@ -358,9 +359,13 @@ static inline void sb_queue_tail(struct netns_ipvs *ipvs)
struct ip_vs_sync_buff *sb = ipvs->sync_buff;
spin_lock(&ipvs->sync_lock);
- if (ipvs->sync_state & IP_VS_STATE_MASTER)
+ if (ipvs->sync_state & IP_VS_STATE_MASTER &&
+ ipvs->sync_queue_len < sysctl_sync_qlen_max(ipvs)) {
list_add_tail(&sb->list, &ipvs->sync_queue);
- else
+ ipvs->sync_queue_len++;
+ if (!((++ipvs->sync_queue_sent) & (IPVS_SYNC_WAKEUP_RATE-1)))
+ wake_up_process(ipvs->master_thread);
+ } else
ip_vs_sync_buff_release(sb);
spin_unlock(&ipvs->sync_lock);
}
@@ -405,10 +410,11 @@ void ip_vs_sync_switch_mode(struct net *net, int mode)
ipvs->sync_buff = NULL;
} else {
spin_lock_bh(&ipvs->sync_lock);
- if (ipvs->sync_state & IP_VS_STATE_MASTER)
+ if (ipvs->sync_state & IP_VS_STATE_MASTER) {
list_add_tail(&ipvs->sync_buff->list,
&ipvs->sync_queue);
- else
+ ipvs->sync_queue_len++;
+ } else
ip_vs_sync_buff_release(ipvs->sync_buff);
spin_unlock_bh(&ipvs->sync_lock);
}
@@ -1392,18 +1398,22 @@ ip_vs_send_async(struct socket *sock, const char *buffer, const size_t length)
return len;
}
-static void
+static int
ip_vs_send_sync_msg(struct socket *sock, struct ip_vs_sync_mesg *msg)
{
int msize;
+ int ret;
msize = msg->size;
/* Put size in network byte order */
msg->size = htons(msg->size);
- if (ip_vs_send_async(sock, (char *)msg, msize) != msize)
- pr_err("ip_vs_send_async error\n");
+ ret = ip_vs_send_async(sock, (char *)msg, msize);
+ if (ret >= 0 || ret == -EAGAIN)
+ return ret;
+ pr_err("ip_vs_send_async error %d\n", ret);
+ return 0;
}
static int
@@ -1428,32 +1438,58 @@ ip_vs_receive(struct socket *sock, char *buffer, const size_t buflen)
return len;
}
+/* Get next buffer to send */
+static inline struct ip_vs_sync_buff *
+next_sync_buff(struct netns_ipvs *ipvs)
+{
+ struct ip_vs_sync_buff *sb;
+
+ sb = sb_dequeue(ipvs);
+ if (sb)
+ return sb;
+ /* Do not delay entries in buffer for more than 2 seconds */
+ return get_curr_sync_buff(ipvs, 2 * HZ);
+}
static int sync_thread_master(void *data)
{
struct ip_vs_sync_thread_data *tinfo = data;
struct netns_ipvs *ipvs = net_ipvs(tinfo->net);
+ struct sock *sk = tinfo->sock->sk;
struct ip_vs_sync_buff *sb;
pr_info("sync thread started: state = MASTER, mcast_ifn = %s, "
"syncid = %d\n",
ipvs->master_mcast_ifn, ipvs->master_syncid);
- while (!kthread_should_stop()) {
- while ((sb = sb_dequeue(ipvs))) {
- ip_vs_send_sync_msg(tinfo->sock, sb->mesg);
- ip_vs_sync_buff_release(sb);
+ for (;;) {
+ sb = next_sync_buff(ipvs);
+ if (!sb) {
+ set_current_state(TASK_INTERRUPTIBLE);
+ if (kthread_should_stop())
+ break;
+ schedule_timeout(HZ / 5);
+ continue;
}
- /* check if entries stay in ipvs->sync_buff for 2 seconds */
- sb = get_curr_sync_buff(ipvs, 2 * HZ);
- if (sb) {
- ip_vs_send_sync_msg(tinfo->sock, sb->mesg);
- ip_vs_sync_buff_release(sb);
+retry:
+ if (unlikely(kthread_should_stop()))
+ break;
+ if (ip_vs_send_sync_msg(tinfo->sock, sb->mesg) < 0) {
+ int ret = 0;
+
+ __wait_event_interruptible(*sk_sleep(sk),
+ sock_writeable(sk) ||
+ kthread_should_stop(),
+ ret);
+ goto retry;
}
^ permalink raw reply related
* ppp: Fix race condition with queue start/stop
From: David Woodhouse @ 2012-04-08 20:01 UTC (permalink / raw)
To: David Miller; +Cc: paulus, netdev
In-Reply-To: <1333915104.31812.61.camel@shinybook.infradead.org>
Commit e675f0cc9a872fd152edc0c77acfed19bf28b81e ("ppp: Don't stop and
restart queue on every TX packet") introduced a race condition which
could leave the net queue stopped even when the channel is no longer
busy. By calling netif_stop_queue() from ppp_start_xmit(), based on the
return value from ppp_xmit_process() but *after* all the locks have been
dropped, we could potentially do so *after* the channel has actually
finished transmitting and attempted to re-wake the queue.
Fix this by moving the netif_stop_queue() into ppp_xmit_process() under
the xmit lock. I hadn't done this previously, because it gets called
from other places than ppp_start_xmit(). But I now think it's the better
option. The net queue *should* be stopped if the channel becomes
congested due to writes from pppd, anyway.
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
drivers/net/ppp/ppp_generic.c | 15 ++++++---------
1 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ppp/ppp_generic.c b/drivers/net/ppp/ppp_generic.c
index 33f8c51..21d7151 100644
--- a/drivers/net/ppp/ppp_generic.c
+++ b/drivers/net/ppp/ppp_generic.c
@@ -235,7 +235,7 @@ struct ppp_net {
/* Prototypes. */
static int ppp_unattached_ioctl(struct net *net, struct ppp_file *pf,
struct file *file, unsigned int cmd, unsigned long arg);
-static int ppp_xmit_process(struct ppp *ppp);
+static void ppp_xmit_process(struct ppp *ppp);
static void ppp_send_frame(struct ppp *ppp, struct sk_buff *skb);
static void ppp_push(struct ppp *ppp);
static void ppp_channel_push(struct channel *pch);
@@ -969,8 +969,7 @@ ppp_start_xmit(struct sk_buff *skb, struct net_device *dev)
put_unaligned_be16(proto, pp);
skb_queue_tail(&ppp->file.xq, skb);
- if (!ppp_xmit_process(ppp))
- netif_stop_queue(dev);
+ ppp_xmit_process(ppp);
return NETDEV_TX_OK;
outf:
@@ -1048,11 +1047,10 @@ static void ppp_setup(struct net_device *dev)
* Called to do any work queued up on the transmit side
* that can now be done.
*/
-static int
+static void
ppp_xmit_process(struct ppp *ppp)
{
struct sk_buff *skb;
- int ret = 0;
ppp_xmit_lock(ppp);
if (!ppp->closing) {
@@ -1062,13 +1060,12 @@ ppp_xmit_process(struct ppp *ppp)
ppp_send_frame(ppp, skb);
/* If there's no work left to do, tell the core net
code that we can accept some more. */
- if (!ppp->xmit_pending && !skb_peek(&ppp->file.xq)) {
+ if (!ppp->xmit_pending && !skb_peek(&ppp->file.xq))
netif_wake_queue(ppp->dev);
- ret = 1;
- }
+ else
+ netif_stop_queue(ppp->dev);
}
ppp_xmit_unlock(ppp);
- return ret;
}
static inline struct sk_buff *
--
1.7.7.6
--
dwmw2
^ permalink raw reply related
* Re: [PATCH] ppp: Don't stop and restart queue on every TX packet
From: David Woodhouse @ 2012-04-08 19:58 UTC (permalink / raw)
To: David Miller; +Cc: paulus, netdev
In-Reply-To: <20120403.172958.144419545310336164.davem@davemloft.net>
[-- Attachment #1: Type: text/plain, Size: 1702 bytes --]
On Tue, 2012-04-03 at 17:29 -0400, David Miller wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> Date: Mon, 26 Mar 2012 11:03:42 +0100
>
> > For every transmitted packet, ppp_start_xmit() will stop the netdev
> > queue and then, if appropriate, restart it. This causes the TX softirq
> > to run, entirely gratuitously.
> >
> > This is "only" a waste of CPU time in the normal case, but it's actively
> > harmful when the PPP device is a TEQL slave ― the wakeup will cause the
> > offending device to receive the next TX packet from the TEQL queue, when
> > it *should* have gone to the next slave in the list. We end up seeing
> > large bursts of packets on just *one* slave device, rather than using
> > the full available bandwidth over all slaves.
> >
> > This patch fixes the problem by *not* unconditionally stopping the queue
> > in ppp_start_xmit(). It adds a return value from ppp_xmit_process()
> > which indicates whether the queue should be stopped or not.
> >
> > It *doesn't* remove the call to netif_wake_queue() from
> > ppp_xmit_process(), because other code paths (especially from
> > ppp_output_wakeup()) need it there and it's messy to push it out to the
> > other callers to do it based on the return value. So we leave it in
> > place ― it's a no-op in the case where the queue wasn't stopped, so it's
> > harmless in the TX path.
> >
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>
> Applied, thanks.
Hmm. After going through the locking issues on wakeup, for the PPPoATM
patch I just sent, I think that the above has introduced a bug. I'll
send a patch with a full explanation in a few moments...
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]
^ permalink raw reply
* [PATCH] pppoatm: Fix excessive queue bloat
From: David Woodhouse @ 2012-04-08 19:55 UTC (permalink / raw)
To: netdev; +Cc: David Miller, paulus, chas williams - CONTRACTOR, Eric Dumazet
In-Reply-To: <1332880522.2058.67.camel@shinybook.infradead.org>
[-- Attachment #1: Type: text/plain, Size: 7951 bytes --]
We discovered that PPPoATM has an excessively deep transmit queue. A
queue the size of the default socket send buffer (wmem_default) is
maintained between the PPP generic core and the ATM device.
Fix it to queue a maximum of *two* packets. The one the ATM device is
currently working on, and one more for the ATM driver to process
immediately in its TX done interrupt handler. The PPP core is designed
to feed packets to the channel with minimal latency, so that really
ought to be enough to keep the ATM device busy.
While we're at it, fix the fact that we were triggering the wakeup
tasklet on *every* pppoatm_pop() call. The comment saying "this is
inefficient, but doing it right is too hard" turns out to be overly
pessimistic... I think :)
On machines like the Traverse Geos, with a slow Geode CPU and two
high-speed ADSL2+ interfaces, there were reports of extremely high CPU
usage which could partly be attributed to the extra wakeups.
(The wakeup handling could actually be made a whole lot easier if we
stop checking sk->sk_sndbuf altogether. Given that we now only queue
*two* packets ever, one wonders what the point is. As it is, you could
already deadlock the thing by setting the sk_sndbuf to a value lower
than the MTU of the device, and it'd just block for ever.)
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
Seriously, this gets *much* easier if we just ditch the checks against
sk_sndbuf. We just wake up whenever decrementing ->inflight from zero.
Can I?
net/atm/pppoatm.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 85 insertions(+), 10 deletions(-)
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 614d3fc..68a02a9 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -62,12 +62,25 @@ struct pppoatm_vcc {
void (*old_pop)(struct atm_vcc *, struct sk_buff *);
/* keep old push/pop for detaching */
enum pppoatm_encaps encaps;
+ atomic_t inflight;
+ unsigned long blocked;
int flags; /* SC_COMP_PROT - compress protocol */
struct ppp_channel chan; /* interface to generic ppp layer */
struct tasklet_struct wakeup_tasklet;
};
/*
+ * We want to allow two packets in the queue. The one that's currently in
+ * flight, and *one* queued up ready for the ATM device to send immediately
+ * from its TX done IRQ. We want to be able to use atomic_inc_not_zero(), so
+ * inflight == -2 represents an empty queue, -1 one packet, and zero means
+ * there are two packets in the queue.
+ */
+#define NONE_INFLIGHT -2
+
+#define BLOCKED 0
+
+/*
* Header used for LLC Encapsulated PPP (4 bytes) followed by the LCP protocol
* ID (0xC021) used in autodetection
*/
@@ -102,16 +115,30 @@ static void pppoatm_wakeup_sender(unsigned long arg)
static void pppoatm_pop(struct atm_vcc *atmvcc, struct sk_buff *skb)
{
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
pvcc->old_pop(atmvcc, skb);
+ atomic_dec(&pvcc->inflight);
+
/*
- * We don't really always want to do this since it's
- * really inefficient - it would be much better if we could
- * test if we had actually throttled the generic layer.
- * Unfortunately then there would be a nasty SMP race where
- * we could clear that flag just as we refuse another packet.
- * For now we do the safe thing.
+ * We always used to run the wakeup tasklet unconditionally here, for
+ * fear of race conditions where we clear the BLOCKED flag just as we
+ * refuse another packet in pppoatm_send(). This was quite inefficient.
+ *
+ * In fact it's OK. The PPP core will only ever call pppoatm_send()
+ * while holding the channel->downl lock. And ppp_output_wakeup() as
+ * called by the tasklet will *also* grab that lock. So even if another
+ * CPU is in pppoatm_send() right now, the tasklet isn't going to race
+ * with it. The wakeup *will* happen after the other CPU is safely out
+ * of pppoatm_send() again.
+ *
+ * So if the CPU in pppoatm_send() has already set the BLOCKED bit and
+ * it about to return, that's fine. We trigger a wakeup which will
+ * happen later. And if the CPU in pppoatm_send() *hasn't* set the
+ * BLOCKED bit yet, that's fine too because of the double check in
+ * pppoatm_may_send() which is commented there.
*/
- tasklet_schedule(&pvcc->wakeup_tasklet);
+ if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
+ tasklet_schedule(&pvcc->wakeup_tasklet);
}
/*
@@ -184,6 +211,51 @@ error:
ppp_input_error(&pvcc->chan, 0);
}
+static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
+{
+ /*
+ * It's not clear that we need to bother with using atm_may_send()
+ * to check we don't exceed sk->sk_sndbuf. If userspace sets a
+ * value of sk_sndbuf which is lower than the MTU, we're going to
+ * block for ever. But the code always did that before we introduced
+ * the packet count limit, so...
+ */
+ if (atm_may_send(pvcc->atmvcc, size) &&
+ atomic_inc_not_zero_hint(&pvcc->inflight, NONE_INFLIGHT))
+ return 1;
+
+ /*
+ * We use test_and_set_bit() rather than set_bit() here because
+ * we need to ensure there's a memory barrier after it. The bit
+ * *must* be set before we do the atomic_inc() on pvcc->inflight.
+ * There's no smp_mb__after_set_bit(), so it's this or abuse
+ * smp_mb__after_clear_bit().
+ */
+ test_and_set_bit(BLOCKED, &pvcc->blocked);
+
+ /*
+ * We may have raced with pppoatm_pop(). If it ran for the
+ * last packet in the queue, *just* before we set the BLOCKED
+ * bit, then it might never run again and the channel could
+ * remain permanently blocked. Cope with that race by checking
+ * *again*. If it did run in that window, we'll have space on
+ * the queue now and can return success. It's harmless to leave
+ * the BLOCKED flag set, since it's only used as a trigger to
+ * run the wakeup tasklet. Another wakeup will never hurt.
+ * If pppoatm_pop() is running but hasn't got as far as making
+ * space on the queue yet, then it hasn't checked the BLOCKED
+ * flag yet either, so we're safe in that case too. It'll issue
+ * an "immediate" wakeup... where "immediate" actually involves
+ * taking the PPP channel's ->downl lock, which is held by the
+ * code path that calls pppoatm_send(), and is thus going to
+ * wait for us to finish.
+ */
+ if (atm_may_send(pvcc->atmvcc, size) &&
+ atomic_inc_not_zero(&pvcc->inflight))
+ return 1;
+
+ return 0;
+}
/*
* Called by the ppp_generic.c to send a packet - returns true if packet
* was accepted. If we return false, then it's our job to call
@@ -207,7 +279,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
struct sk_buff *n;
n = skb_realloc_headroom(skb, LLC_LEN);
if (n != NULL &&
- !atm_may_send(pvcc->atmvcc, n->truesize)) {
+ !pppoatm_may_send(pvcc, n->truesize)) {
kfree_skb(n);
goto nospace;
}
@@ -215,12 +287,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
skb = n;
if (skb == NULL)
return DROP_PACKET;
- } else if (!atm_may_send(pvcc->atmvcc, skb->truesize))
+ } else if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
break;
case e_vc:
- if (!atm_may_send(pvcc->atmvcc, skb->truesize))
+ if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
break;
case e_autodetect:
@@ -285,6 +357,9 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
if (pvcc == NULL)
return -ENOMEM;
pvcc->atmvcc = atmvcc;
+
+ /* Maximum is zero, so that we can use atomic_inc_not_zero() */
+ atomic_set(&pvcc->inflight, NONE_INFLIGHT);
pvcc->old_push = atmvcc->push;
pvcc->old_pop = atmvcc->pop;
pvcc->encaps = (enum pppoatm_encaps) be.encaps;
--
1.7.7.6
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]
^ permalink raw reply related
* [PATCH] pppoatm: Fix excessive queue bloat
From: David Woodhouse @ 2012-04-08 19:53 UTC (permalink / raw)
To: netdev; +Cc: David Miller, paulus, chas williams - CONTRACTOR, Eric Dumazet
In-Reply-To: <1332880522.2058.67.camel@shinybook.infradead.org>
[-- Attachment #1: Type: text/plain, Size: 7951 bytes --]
We discovered that PPPoATM has an excessively deep transmit queue. A
queue the size of the default socket send buffer (wmem_default) is
maintained between the PPP generic core and the ATM device.
Fix it to queue a maximum of *two* packets. The one the ATM device is
currently working on, and one more for the ATM driver to process
immediately in its TX done interrupt handler. The PPP core is designed
to feed packets to the channel with minimal latency, so that really
ought to be enough to keep the ATM device busy.
While we're at it, fix the fact that we were triggering the wakeup
tasklet on *every* pppoatm_pop() call. The comment saying "this is
inefficient, but doing it right is too hard" turns out to be overly
pessimistic... I think :)
On machines like the Traverse Geos, with a slow Geode CPU and two
high-speed ADSL2+ interfaces, there were reports of extremely high CPU
usage which could partly be attributed to the extra wakeups.
(The wakeup handling could actually be made a whole lot easier if we
stop checking sk->sk_sndbuf altogether. Given that we now only queue
*two* packets ever, one wonders what the point is. As it is, you could
already deadlock the thing by setting the sk_sndbuf to a value lower
than the MTU of the device, and it'd just block for ever.)
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
---
Seriously, this gets *much* easier if we just ditch the checks against
sk_sndbuf. We just wake up whenever decrementing ->inflight from zero.
Can I?
net/atm/pppoatm.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 files changed, 85 insertions(+), 10 deletions(-)
diff --git a/net/atm/pppoatm.c b/net/atm/pppoatm.c
index 614d3fc..68a02a9 100644
--- a/net/atm/pppoatm.c
+++ b/net/atm/pppoatm.c
@@ -62,12 +62,25 @@ struct pppoatm_vcc {
void (*old_pop)(struct atm_vcc *, struct sk_buff *);
/* keep old push/pop for detaching */
enum pppoatm_encaps encaps;
+ atomic_t inflight;
+ unsigned long blocked;
int flags; /* SC_COMP_PROT - compress protocol */
struct ppp_channel chan; /* interface to generic ppp layer */
struct tasklet_struct wakeup_tasklet;
};
/*
+ * We want to allow two packets in the queue. The one that's currently in
+ * flight, and *one* queued up ready for the ATM device to send immediately
+ * from its TX done IRQ. We want to be able to use atomic_inc_not_zero(), so
+ * inflight == -2 represents an empty queue, -1 one packet, and zero means
+ * there are two packets in the queue.
+ */
+#define NONE_INFLIGHT -2
+
+#define BLOCKED 0
+
+/*
* Header used for LLC Encapsulated PPP (4 bytes) followed by the LCP protocol
* ID (0xC021) used in autodetection
*/
@@ -102,16 +115,30 @@ static void pppoatm_wakeup_sender(unsigned long arg)
static void pppoatm_pop(struct atm_vcc *atmvcc, struct sk_buff *skb)
{
struct pppoatm_vcc *pvcc = atmvcc_to_pvcc(atmvcc);
+
pvcc->old_pop(atmvcc, skb);
+ atomic_dec(&pvcc->inflight);
+
/*
- * We don't really always want to do this since it's
- * really inefficient - it would be much better if we could
- * test if we had actually throttled the generic layer.
- * Unfortunately then there would be a nasty SMP race where
- * we could clear that flag just as we refuse another packet.
- * For now we do the safe thing.
+ * We always used to run the wakeup tasklet unconditionally here, for
+ * fear of race conditions where we clear the BLOCKED flag just as we
+ * refuse another packet in pppoatm_send(). This was quite inefficient.
+ *
+ * In fact it's OK. The PPP core will only ever call pppoatm_send()
+ * while holding the channel->downl lock. And ppp_output_wakeup() as
+ * called by the tasklet will *also* grab that lock. So even if another
+ * CPU is in pppoatm_send() right now, the tasklet isn't going to race
+ * with it. The wakeup *will* happen after the other CPU is safely out
+ * of pppoatm_send() again.
+ *
+ * So if the CPU in pppoatm_send() has already set the BLOCKED bit and
+ * it about to return, that's fine. We trigger a wakeup which will
+ * happen later. And if the CPU in pppoatm_send() *hasn't* set the
+ * BLOCKED bit yet, that's fine too because of the double check in
+ * pppoatm_may_send() which is commented there.
*/
- tasklet_schedule(&pvcc->wakeup_tasklet);
+ if (test_and_clear_bit(BLOCKED, &pvcc->blocked))
+ tasklet_schedule(&pvcc->wakeup_tasklet);
}
/*
@@ -184,6 +211,51 @@ error:
ppp_input_error(&pvcc->chan, 0);
}
+static inline int pppoatm_may_send(struct pppoatm_vcc *pvcc, int size)
+{
+ /*
+ * It's not clear that we need to bother with using atm_may_send()
+ * to check we don't exceed sk->sk_sndbuf. If userspace sets a
+ * value of sk_sndbuf which is lower than the MTU, we're going to
+ * block for ever. But the code always did that before we introduced
+ * the packet count limit, so...
+ */
+ if (atm_may_send(pvcc->atmvcc, size) &&
+ atomic_inc_not_zero_hint(&pvcc->inflight, NONE_INFLIGHT))
+ return 1;
+
+ /*
+ * We use test_and_set_bit() rather than set_bit() here because
+ * we need to ensure there's a memory barrier after it. The bit
+ * *must* be set before we do the atomic_inc() on pvcc->inflight.
+ * There's no smp_mb__after_set_bit(), so it's this or abuse
+ * smp_mb__after_clear_bit().
+ */
+ test_and_set_bit(BLOCKED, &pvcc->blocked);
+
+ /*
+ * We may have raced with pppoatm_pop(). If it ran for the
+ * last packet in the queue, *just* before we set the BLOCKED
+ * bit, then it might never run again and the channel could
+ * remain permanently blocked. Cope with that race by checking
+ * *again*. If it did run in that window, we'll have space on
+ * the queue now and can return success. It's harmless to leave
+ * the BLOCKED flag set, since it's only used as a trigger to
+ * run the wakeup tasklet. Another wakeup will never hurt.
+ * If pppoatm_pop() is running but hasn't got as far as making
+ * space on the queue yet, then it hasn't checked the BLOCKED
+ * flag yet either, so we're safe in that case too. It'll issue
+ * an "immediate" wakeup... where "immediate" actually involves
+ * taking the PPP channel's ->downl lock, which is held by the
+ * code path that calls pppoatm_send(), and is thus going to
+ * wait for us to finish.
+ */
+ if (atm_may_send(pvcc->atmvcc, size) &&
+ atomic_inc_not_zero(&pvcc->inflight))
+ return 1;
+
+ return 0;
+}
/*
* Called by the ppp_generic.c to send a packet - returns true if packet
* was accepted. If we return false, then it's our job to call
@@ -207,7 +279,7 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
struct sk_buff *n;
n = skb_realloc_headroom(skb, LLC_LEN);
if (n != NULL &&
- !atm_may_send(pvcc->atmvcc, n->truesize)) {
+ !pppoatm_may_send(pvcc, n->truesize)) {
kfree_skb(n);
goto nospace;
}
@@ -215,12 +287,12 @@ static int pppoatm_send(struct ppp_channel *chan, struct sk_buff *skb)
skb = n;
if (skb == NULL)
return DROP_PACKET;
- } else if (!atm_may_send(pvcc->atmvcc, skb->truesize))
+ } else if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
memcpy(skb_push(skb, LLC_LEN), pppllc, LLC_LEN);
break;
case e_vc:
- if (!atm_may_send(pvcc->atmvcc, skb->truesize))
+ if (!pppoatm_may_send(pvcc, skb->truesize))
goto nospace;
break;
case e_autodetect:
@@ -285,6 +357,9 @@ static int pppoatm_assign_vcc(struct atm_vcc *atmvcc, void __user *arg)
if (pvcc == NULL)
return -ENOMEM;
pvcc->atmvcc = atmvcc;
+
+ /* Maximum is zero, so that we can use atomic_inc_not_zero() */
+ atomic_set(&pvcc->inflight, NONE_INFLIGHT);
pvcc->old_push = atmvcc->push;
pvcc->old_pop = atmvcc->pop;
pvcc->encaps = (enum pppoatm_encaps) be.encaps;
--
1.7.7.6
--
dwmw2
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5818 bytes --]
^ permalink raw reply related
* Re: [PATCH v17 08/15] seccomp: add system call filtering using BPF
From: Indan Zupancic @ 2012-04-08 18:22 UTC (permalink / raw)
To: Andrew Morton
Cc: Will Drewry, linux-kernel, linux-security-module, linux-arch,
linux-doc, kernel-hardening, netdev, x86, arnd, davem, hpa, mingo,
oleg, peterz, rdunlap, mcgrathr, tglx, luto, eparis, serge.hallyn,
djm, scarybeasts, pmoore, corbet, eric.dumazet, markus, coreyb,
keescook, jmorris
In-Reply-To: <20120406132330.457a05cf.akpm@linux-foundation.org>
On Sat, April 7, 2012 06:23, Andrew Morton wrote:
> hm, I'm surprised that we don't have a zero-returning implementation of
> is_compat_task() when CONFIG_COMPAT=n. Seems silly. Blames Arnd.
It's sneakily hidden at the end of compat.h.
>> +/**
>> + * get_u32 - returns a u32 offset into data
>> + * @data: a unsigned 64 bit value
>> + * @index: 0 or 1 to return the first or second 32-bits
>> + *
>> + * This inline exists to hide the length of unsigned long.
>> + * If a 32-bit unsigned long is passed in, it will be extended
>> + * and the top 32-bits will be 0. If it is a 64-bit unsigned
>> + * long, then whatever data is resident will be properly returned.
>> + */
>> +static inline u32 get_u32(u64 data, int index)
>> +{
>> + return ((u32 *)&data)[index];
>> +}
>
> This seems utterly broken on big-endian machines. If so: fix. If not:
> add comment explaining why?
It's not a bug, it's intentional.
I tried to convince them to have a stable ABI for all archs, but they
didn't want to make the ABI endianness independent, because it looks
uglier. The argument being that system call numbers are arch dependent
anyway.
So a filter for a little endian arch needs to check a different offset
than one for a big endian archs.
>> +static u32 seccomp_run_filters(int syscall)
>> +{
>> + struct seccomp_filter *f;
>> + u32 ret = SECCOMP_RET_KILL;
>> + /*
>> + * All filters are evaluated in order of youngest to oldest. The lowest
>> + * BPF return value always takes priority.
>> + */
>
> The youngest-first design surprised me. It wasn't mentioned at all in
> the changelog. Thinking about it, I guess it just doesn't matter. But
> some description of the reasons for and implications of this decision
> for the uninitiated would be welcome.
I think it's less confusing to not mention the order at all, exactly
because it doesn't matter. It has been like this from the start, so
that's why it's not mentioned in the changelog I guess.
The reason to check the youngest first is because the filters are shared
between processes: childs inherit it. If later on additional filters are
added, the only way of adding them without modifying an older filter is
by adding them to the head of the list. This way no locking is needed,
because filters are only added and removed single-threadedly, and never
modified when being shared.
>> +/**
>> + * seccomp_attach_filter: Attaches a seccomp filter to current.
>> + * @fprog: BPF program to install
>> + *
>> + * Returns 0 on success or an errno on failure.
>> + */
>> +static long seccomp_attach_filter(struct sock_fprog *fprog)
>> +{
>> + struct seccomp_filter *filter;
>> + unsigned long fp_size = fprog->len * sizeof(struct sock_filter);
>> + unsigned long total_insns = fprog->len;
>> + long ret;
>> +
>> + if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
>> + return -EINVAL;
>> +
>> + for (filter = current->seccomp.filter; filter; filter = filter->prev)
>> + total_insns += filter->len + 4; /* include a 4 instr penalty */
>
> So tasks don't share filters? We copy them by value at fork? Do we do
> this at vfork() too?
Yes they do. But shared filters are never modified, except for the refcount.
>
>> + if (total_insns > MAX_INSNS_PER_PATH)
>> + return -ENOMEM;
>> +
>> + /*
>> + * Installing a seccomp filter requires that the task have
>> + * CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
>> + * This avoids scenarios where unprivileged tasks can affect the
>> + * behavior of privileged children.
>> + */
>> + if (!current->no_new_privs &&
>> + security_capable_noaudit(current_cred(), current_user_ns(),
>> + CAP_SYS_ADMIN) != 0)
>> + return -EACCES;
>> +
>> + /* Allocate a new seccomp_filter */
>> + filter = kzalloc(sizeof(struct seccomp_filter) + fp_size, GFP_KERNEL);
>
> I think this gives userspace an easy way of causing page allocation
> failure warnings, by permitting large kmalloc() attempts. Add
> __GFP_NOWARN?
Max is 32kb. sk_attach_filter() in net/core/filter.c is worse,
it allocates up to 512kb before even checking the length.
What about using GFP_USER (and adding __GFP_NOWARN to GFP_USER) instead?
>> + /* Check and rewrite the fprog via the skb checker */
>> + ret = sk_chk_filter(filter->insns, filter->len);
>> + if (ret)
>> + goto fail;
>> +
>> + /* Check and rewrite the fprog for seccomp use */
>> + ret = seccomp_chk_filter(filter->insns, filter->len);
>
> "check" is spelled "check"!
Yes, it is and he did spell "check" as "Check".
seccomp_chk_filter() mirrors sk_chk_filter(). So it refers to
"chk", not "check".
>
>> + if (ret)
>> + goto fail;
>> +
>> + /*
>> + * If there is an existing filter, make it the prev and don't drop its
>> + * task reference.
>> + */
>> + filter->prev = current->seccomp.filter;
>> + current->seccomp.filter = filter;
>> + return 0;
>> +fail:
>> + kfree(filter);
>> + return ret;
>> +}
>> +
>>
>> ...
>>
>> +/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
>> +void put_seccomp_filter(struct task_struct *tsk)
>> +{
>> + struct seccomp_filter *orig = tsk->seccomp.filter;
>> + /* Clean up single-reference branches iteratively. */
>> + while (orig && atomic_dec_and_test(&orig->usage)) {
>> + struct seccomp_filter *freeme = orig;
>> + orig = orig->prev;
>> + kfree(freeme);
>> + }
>> +}
>
> So if one of the filters in the list has an elevated refcount, we bail
> out on the remainder of the list. Seems odd.
A bit odd yes, but fiddling with the other refcounts makes no functional
difference. You can see it as a tree of filters, with a fork at each
point any process forked, and the filters between forks having the same
life-time (added by the same process). Every process's filter list is
a path from a leaf filter to the root.
Only leaf filters get their refcount incremented at fork time, so their
refcount is always the same or higher than the refcount of the filters
before them, up to the previous fork. The intermediate filters always
have a refcount of 1, real refcounting only happens on the fork filters.
A refcount of 1 means it's an intermediate or a leaf filter, a higher
refcount means it's a fork filter.
With "proper" refcounting all the filters from the new leaf up to the
root would get their refcount incremented at fork time, but that is
wasted work, because they can be guarded by the last filter instead.
So the code bails when reaching a fork point, as it means that filter
and hence the rest of the list is shared and used by other processes.
Greetings,
Indan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox