* Re: [PATCH 2/2] bonding: COW before overwriting the destination MAC address
From: Changli Gao @ 2011-03-03 6:54 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299134742.2456.1.camel@edumazet-laptop>
On Thu, Mar 3, 2011 at 2:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 03 mars 2011 à 12:25 +0800, Changli Gao a écrit :
>> When there is a ptype handler holding a clone of this skb, whose
>> destination MAC addresse is overwritten, the owner of this handler may
>> get a corrupted packet.
>>
>> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
>> ---
>> drivers/net/bonding/bond_main.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index 912b416..211be52 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1513,6 +1513,11 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
>> skb->pkt_type == PACKET_HOST) {
>> u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
>>
>> + if (unlikely(skb_cow_head(skb,
>> + skb->data - skb_mac_header(skb)))) {
>> + kfree_skb(skb);
>> + return NULL;
>> + }
>> memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
>> }
>>
>
> This seems buggy.
>
> dest points to old skb head content.
>
>
Oh, thanks. I'll respin it.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Changli Gao @ 2011-03-03 7:07 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: David S. Miller, Eric Dumazet, netdev, Changli Gao
When there is a ptype handler holding a clone of this skb, whose
destination MAC addresse is overwritten, the owner of this handler may
get a corrupted packet.
Signed-off-by: Changli Gao <xiaosuo@gmail.com>
---
v2: fix the bug in the previous one. Thank him.
drivers/net/bonding/bond_main.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
index 912b416..7b7ca97 100644
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1511,9 +1511,13 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
if (bond_dev->priv_flags & IFF_MASTER_ALB &&
bond_dev->priv_flags & IFF_BRIDGE_PORT &&
skb->pkt_type == PACKET_HOST) {
- u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
- memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
+ if (unlikely(skb_cow_head(skb,
+ skb->data - skb_mac_header(skb)))) {
+ kfree_skb(skb);
+ return NULL;
+ }
+ memcpy(eth_hdr(skb)->h_dest, bond_dev->dev_addr, ETH_ALEN);
}
return skb;
^ permalink raw reply related
* Re: Kernel panic nf_nat_setup_info+0x5b3/0x6e0
From: Changli Gao @ 2011-03-03 7:33 UTC (permalink / raw)
To: Oleg A. Arkhangelsky
Cc: Patrick McHardy, netfilter-devel, netdev, Paul E McKenney
In-Reply-To: <124481299095426@web67.yandex.ru>
[-- Attachment #1: Type: text/plain, Size: 1461 bytes --]
On Thu, Mar 3, 2011 at 3:50 AM, "Oleg A. Arkhangelsky"
<sysoleg@yandex.ru> wrote:
> 02.03.2011, 17:37, "Changli Gao" <xiaosuo@gmail.com>:
>
>> t should be NULL here, as offsetof(struct nf_conn, dst.protonum) == 0x36.
>> We should free the nf_ct_extend with call_rcu(), since nat ext is
>> referenced in the rcu read context.
>
> Yes, I think the problem is triggered when nf_conntrack_free() is called by
> different CPU during net->ipv4.nat_bysource hash traversal. Extensions
> framework doesn't have any SLAB_DESTROY_BY_RCU magic.
>
> I'm not sure, but couldn't this problem be introduced by:
>
> ea781f197d6a835cbb93a0bf88ee1696296ed8aa
> netfilter: nf_conntrack: use SLAB_DESTROY_BY_RCU and get rid of call_rcu()
>
> ?
>
There is nothing to do with SLAB_DESTROY_BY_RCU. Here is the comment
for this flag:
/*
* SLAB_DESTROY_BY_RCU - **WARNING** READ THIS!
*
* This delays freeing the SLAB page by a grace period, it does _NOT_
* delay object freeing. This means that if you do kmem_cache_free()
* that memory location is free to be reused at any time. Thus it may
* be possible to see another object there in the same RCU grace period.
*
* This feature only ensures the memory location backing the object
* stays valid, the trick to using this is relying on an independent
* object validation pass. Something like:
...
Please try the patch attached and test if the problem is solved or not. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
[-- Attachment #2: nf_ext_rcu.diff --]
[-- Type: text/plain, Size: 1384 bytes --]
diff --git a/include/net/netfilter/nf_conntrack_extend.h b/include/net/netfilter/nf_conntrack_extend.h
index 2dcf317..354cccb9 100644
--- a/include/net/netfilter/nf_conntrack_extend.h
+++ b/include/net/netfilter/nf_conntrack_extend.h
@@ -66,13 +66,15 @@ static inline void nf_ct_ext_destroy(struct nf_conn *ct)
__nf_ct_ext_destroy(ct);
}
+void __nf_ct_ext_free_rcu(struct rcu_head *head);
+
/* Free operation. If you want to free a object referred from private area,
* please implement __nf_ct_ext_free() and call it.
*/
static inline void nf_ct_ext_free(struct nf_conn *ct)
{
if (ct->ext)
- kfree(ct->ext);
+ call_rcu(&ct->ext->rcu, __nf_ct_ext_free_rcu);
}
/* Add this type, returns pointer to data or NULL. */
diff --git a/net/netfilter/nf_conntrack_extend.c b/net/netfilter/nf_conntrack_extend.c
index 80a23ed..3a47b76 100644
--- a/net/netfilter/nf_conntrack_extend.c
+++ b/net/netfilter/nf_conntrack_extend.c
@@ -68,11 +68,12 @@ nf_ct_ext_create(struct nf_ct_ext **ext, enum nf_ct_ext_id id, gfp_t gfp)
return (void *)(*ext) + off;
}
-static void __nf_ct_ext_free_rcu(struct rcu_head *head)
+void __nf_ct_ext_free_rcu(struct rcu_head *head)
{
struct nf_ct_ext *ext = container_of(head, struct nf_ct_ext, rcu);
kfree(ext);
}
+EXPORT_SYMBOL_GPL(__nf_ct_ext_free_rcu);
void *__nf_ct_ext_add(struct nf_conn *ct, enum nf_ct_ext_id id, gfp_t gfp)
{
^ permalink raw reply related
* Re: [PATCH 5/7] x25: remove the BKL
From: Andrew Hendry @ 2011-03-03 7:38 UTC (permalink / raw)
To: David Miller; +Cc: arnd, linux-kernel, linux-x25, netdev, eric.dumazet
In-Reply-To: <20110301.151622.200374696.davem@davemloft.net>
Looks good, put 8gig through it over the past few days and system stable.
Tested-by: Andrew Hendry <andrew.hendry@gmail.com>
On Wed, Mar 2, 2011 at 10:16 AM, David Miller <davem@davemloft.net> wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> Date: Wed, 2 Mar 2011 00:13:09 +0100
>
>> This replaces all instances of lock_kernel in x25
>> with lock_sock, taking care to release the socket
>> lock around sleeping functions (sock_alloc_send_skb
>> and skb_recv_datagram). It is not clear whether
>> this is a correct solution, but it seem to be what
>> other protocols do in the same situation.
>>
>> Includes a fix suggested by Eric Dumazet.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Acked-by: David S. Miller <davem@davemloft.net>
>
^ permalink raw reply
* Re: inetpeer with create==0
From: Eric Dumazet @ 2011-03-03 7:39 UTC (permalink / raw)
To: David Miller; +Cc: xiaosuo, netdev
In-Reply-To: <20110302.224220.179921025.davem@davemloft.net>
Le mercredi 02 mars 2011 à 22:42 -0800, David Miller a écrit :
> Actually, back to the original topic, I wonder how bad it is to simply
> elide the recheck in the create==0 case anyways. Except for the ipv4
> fragmentation wraparound protection values, perfect inetpeer finding
> is not necessary for correctness. And IPv4 fragmentation always calls
> inetpeer with create!=0.
>
We could use a seqlock, to detect that a writer might have changed
things while we did our RCU lookup ?
^ permalink raw reply
* Re: [GIT PULL nf-2.6] IPVS
From: Simon Horman @ 2011-03-03 7:47 UTC (permalink / raw)
To: Patrick McHardy
Cc: lvs-devel, netdev, netfilter-devel, netfilter, Hans Schillstrom,
Julian Anastasov
In-Reply-To: <4D6F3AD2.30502@trash.net>
On Thu, Mar 03, 2011 at 07:53:06AM +0100, Patrick McHardy wrote:
> On 02.03.2011 23:06, Simon Horman wrote:
> > On Wed, Mar 02, 2011 at 11:58:18AM +0100, Patrick McHardy wrote:
> >> Am 01.03.2011 23:59, schrieb Simon Horman:
> >>> Hi Patrick,
> >>>
> >>> please consider pulling
> >>> git://git.kernel.org/pub/scm/linux/kernel/git/horms/lvs-test-2.6.git for-patrick
> >>> to get the following change from Julian. Please note that it is an nf-2.6
> >>> (that is 2.6.38-rc) change.
> >>>
> >>> Julian Anastasov (1):
> >>> ipvs: fix dst_lock locking on dest update
> >>
> >> Pulled, thanks Simon.
> >
> > Thanks Patrick.
> >
> > This change is also needed in nf-next-2.6 but I assume that
> > will automatically happen when nf-2.6 is merged into nf-next-2.6,
> > possibly via a merge of net-2.6 into net-next-2.6.
>
> Yes. If you need it as base for further work, the fastest way
> is to ask Dave to pull net-2.6 into net-next-2.6, I can then
> pull it into nf-next.
There is no rush at this time.
^ permalink raw reply
* Re: [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Eric Dumazet @ 2011-03-03 7:55 UTC (permalink / raw)
To: Changli Gao; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299136034-5549-1-git-send-email-xiaosuo@gmail.com>
Le jeudi 03 mars 2011 à 15:07 +0800, Changli Gao a écrit :
> When there is a ptype handler holding a clone of this skb, whose
> destination MAC addresse is overwritten, the owner of this handler may
> get a corrupted packet.
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
> ---
> v2: fix the bug in the previous one. Thank him.
> drivers/net/bonding/bond_main.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 912b416..7b7ca97 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1511,9 +1511,13 @@ static struct sk_buff *bond_handle_frame(struct sk_buff *skb)
> if (bond_dev->priv_flags & IFF_MASTER_ALB &&
> bond_dev->priv_flags & IFF_BRIDGE_PORT &&
> skb->pkt_type == PACKET_HOST) {
> - u16 *dest = (u16 *) eth_hdr(skb)->h_dest;
>
> - memcpy(dest, bond_dev->dev_addr, ETH_ALEN);
> + if (unlikely(skb_cow_head(skb,
> + skb->data - skb_mac_header(skb)))) {
> + kfree_skb(skb);
> + return NULL;
> + }
> + memcpy(eth_hdr(skb)->h_dest, bond_dev->dev_addr, ETH_ALEN);
> }
>
> return skb;
Thats minor, but using :
u16 *dest = eth_hdr(skb)->h_dest;
memcpy(dest, ptr, ETH_ALEN);
Is better because compiler knows both destination and source are at
least aligned on shorts.
On some arches, it helps to not using 6 bytes copy, but 3 shorts.
^ permalink raw reply
* Re: inetpeer with create==0
From: Changli Gao @ 2011-03-03 8:07 UTC (permalink / raw)
To: David Miller; +Cc: eric.dumazet, netdev
In-Reply-To: <20110302.224220.179921025.davem@davemloft.net>
On Thu, Mar 3, 2011 at 2:42 PM, David Miller <davem@davemloft.net> wrote:
>
> Thats why I am working hard to remove it :-)
>
> The %99 percentile performance of our routing cache is absolutely
> terrible. The routing cache does nothing except get in the way.
>
> It is the reason why Linux is still completely unsuitable for use as a
> router on the core internet.
>
After routing cache is removed totally, Linux is still unsuitable for
use as a router on the core internet. Because it isn't a realtime OS,
and the forwarding delay isn't bounded. With routing cache, the memory
cost isn't bounded too. :)
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Fabio Checconi @ 2011-03-03 8:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Stephen Hemminger, David Miller, Luigi Rizzo, netdev,
Paolo Valente
In-Reply-To: <1299087087.2920.27.camel@edumazet-laptop>
Hi,
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, Mar 02, 2011 06:31:27PM +0100
>
> Le mercredi 02 mars 2011 à 17:18 +0100, Eric Dumazet a écrit :
> > Le mercredi 02 mars 2011 à 08:11 -0800, Stephen Hemminger a écrit :
> >
> > > I put the iproute2 code into the repository in the experimental branch.
> > >
> >
> > Thanks
> >
> > It seems as soon as packets are dropped, qdisc is frozen (no more
> > packets dequeued)
I've been able to reproduce this problem using netem, and it seems to
be due to this check:
> + /* If the new skb is not the head of queue, then done here. */
> + if (skb != qdisc_peek_head(cl->qdisc))
> + return err;
changing it to:
if (cl->qdisc->q.qlen != 1)
return err;
seems to make things work. I think this is because qdisc_peek_head()
looks at the wrong list, as the skb is not queued directly in q, but
ends up in the child qdisc attached to cl->qdisc.
> >
> > Hmm...
> >
>
> It seems class deletes are buggy.
>
> After one "tc class del dev $ETH classid 11:1 ..."
>
> a "tc -s -d qdisc show dev $ETH" triggers an Oops
>
This seems to be due to:
> +static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
> +{
> + struct qfq_sched *q = (struct qfq_sched *)sch;
it should be:
struct qfq_sched *q = sched_priv(sch);
The same bug you identified in qfq_reset_qdisc() is present also in
qfq_drop(), both loops need to be corrected...
It should also be noted that this scheduler (like HFSC, IIRC) depends
on the child qdisc not to reorder packets for the guarantees to be met,
as the timestamps need to be in sync with the length of the packet at the
head of the queue. If this can't be guaranteed, to preserve the formal
correctness it should be changed to always use the maximum packet size
to calculate the timestamps.
@Stephen: not that I'm proud of that, but all the bugs found so far are mine...
^ permalink raw reply
* Re: [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Changli Gao @ 2011-03-03 8:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <1299138901.2456.32.camel@edumazet-laptop>
On Thu, Mar 3, 2011 at 3:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
>
> Thats minor, but using :
>
> u16 *dest = eth_hdr(skb)->h_dest;
>
> memcpy(dest, ptr, ETH_ALEN);
>
> Is better because compiler knows both destination and source are at
> least aligned on shorts.
>
> On some arches, it helps to not using 6 bytes copy, but 3 shorts.
>
>
Is it still true if ptr isn't aligned on shorts? And
net_device.dev_addr is an unsigned char *pointer. Thanks.
--
Regards,
Changli Gao(xiaosuo@gmail.com)
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 8:30 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1299135107.2456.7.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 Mar 2011 07:51:47 +0100
> Hmm... I'm curious you send this hack to me ;)
Ok, it's so simple :-)
diff --git a/net/ipv4/inetpeer.c b/net/ipv4/inetpeer.c
index 48f8d45..a194e91 100644
--- a/net/ipv4/inetpeer.c
+++ b/net/ipv4/inetpeer.c
@@ -492,6 +492,8 @@ struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
unlink_from_unused(p);
return p;
}
+ if (!create)
+ return NULL;
/* retry an exact lookup, taking the lock before.
* At least, nodes should be hot in our cache.
@@ -505,7 +507,7 @@ struct inet_peer *inet_getpeer(struct inetpeer_addr *daddr, int create)
unlink_from_unused(p);
return p;
}
- p = create ? kmem_cache_alloc(peer_cachep, GFP_ATOMIC) : NULL;
+ p = kmem_cache_alloc(peer_cachep, GFP_ATOMIC);
if (p) {
p->daddr = *daddr;
atomic_set(&p->refcnt, 1);
^ permalink raw reply related
* Re: [GIT PULL nf-2.6] IPVS
From: David Miller @ 2011-03-03 8:31 UTC (permalink / raw)
To: kaber; +Cc: horms, lvs-devel, netdev, netfilter-devel, netfilter, hans, ja
In-Reply-To: <4D6F3AD2.30502@trash.net>
From: Patrick McHardy <kaber@trash.net>
Date: Thu, 03 Mar 2011 07:53:06 +0100
> Yes. If you need it as base for further work, the fastest way
> is to ask Dave to pull net-2.6 into net-next-2.6, I can then
> pull it into nf-next.
I plan to do a net-2.6 into net-next-2.6 merge for another reason
tomorrow, so this should be taken care of soon.
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 8:32 UTC (permalink / raw)
To: eric.dumazet; +Cc: xiaosuo, netdev
In-Reply-To: <1299137977.2456.15.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 Mar 2011 08:39:37 +0100
> Le mercredi 02 mars 2011 à 22:42 -0800, David Miller a écrit :
>> Actually, back to the original topic, I wonder how bad it is to simply
>> elide the recheck in the create==0 case anyways. Except for the ipv4
>> fragmentation wraparound protection values, perfect inetpeer finding
>> is not necessary for correctness. And IPv4 fragmentation always calls
>> inetpeer with create!=0.
>
> We could use a seqlock, to detect that a writer might have changed
> things while we did our RCU lookup ?
That would certainly work.
^ permalink raw reply
* Re: inetpeer with create==0
From: David Miller @ 2011-03-03 8:34 UTC (permalink / raw)
To: xiaosuo; +Cc: eric.dumazet, netdev
In-Reply-To: <AANLkTinz4ppTi=580K9O=RZ6zbuBrTtjBDhEEnWM3Txg@mail.gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Thu, 3 Mar 2011 16:07:32 +0800
> After routing cache is removed totally, Linux is still unsuitable for
> use as a router on the core internet. Because it isn't a realtime OS,
> and the forwarding delay isn't bounded. With routing cache, the memory
> cost isn't bounded too. :)
With many cores, pipe can be filled no problem, because with multi-core
machine routing lookup proceeds just like Cisco router makes in hardware.
I wrote about this in my blog a few years ago.
^ permalink raw reply
* Re: [PATCH 2/2 v2] bonding: COW before overwriting the destination MAC address
From: Eric Dumazet @ 2011-03-03 8:35 UTC (permalink / raw)
To: Changli Gao; +Cc: Jay Vosburgh, David S. Miller, netdev
In-Reply-To: <AANLkTimNb-ZPFuhhdsSroFzaHKFauR3pTBXL3HMmnECf@mail.gmail.com>
Le jeudi 03 mars 2011 à 16:21 +0800, Changli Gao a écrit :
> On Thu, Mar 3, 2011 at 3:55 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >
> >
> >
> > Thats minor, but using :
> >
> > u16 *dest = eth_hdr(skb)->h_dest;
> >
> > memcpy(dest, ptr, ETH_ALEN);
> >
> > Is better because compiler knows both destination and source are at
> > least aligned on shorts.
> >
> > On some arches, it helps to not using 6 bytes copy, but 3 shorts.
> >
> >
>
> Is it still true if ptr isn't aligned on shorts? And
> net_device.dev_addr is an unsigned char *pointer. Thanks.
>
dev_addr[] was aligned to word boundaries (because of natural structure
alignment), but the recent changes made it a char *pointer, so gcc is
not able to make this true anymore.
This could change if dev_addr was a pointer to struct netdev_hw_addr
^ permalink raw reply
* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Eric Dumazet @ 2011-03-03 8:27 UTC (permalink / raw)
To: Fabio Checconi
Cc: Stephen Hemminger, David Miller, Luigi Rizzo, netdev,
Paolo Valente
In-Reply-To: <20110303081940.GA29685@gandalf.sssup.it>
Le jeudi 03 mars 2011 à 09:19 +0100, Fabio Checconi a écrit :
> Hi,
>
> > From: Eric Dumazet <eric.dumazet@gmail.com>
> > Date: Wed, Mar 02, 2011 06:31:27PM +0100
> >
> > Le mercredi 02 mars 2011 à 17:18 +0100, Eric Dumazet a écrit :
> > > Le mercredi 02 mars 2011 à 08:11 -0800, Stephen Hemminger a écrit :
> > >
> > > > I put the iproute2 code into the repository in the experimental branch.
> > > >
> > >
> > > Thanks
> > >
> > > It seems as soon as packets are dropped, qdisc is frozen (no more
> > > packets dequeued)
>
> I've been able to reproduce this problem using netem, and it seems to
> be due to this check:
>
> > + /* If the new skb is not the head of queue, then done here. */
> > + if (skb != qdisc_peek_head(cl->qdisc))
> > + return err;
>
> changing it to:
>
> if (cl->qdisc->q.qlen != 1)
> return err;
>
> seems to make things work. I think this is because qdisc_peek_head()
> looks at the wrong list, as the skb is not queued directly in q, but
> ends up in the child qdisc attached to cl->qdisc.
>
Strange, I used the default qdisc (pfifo) created in qfq classes
>
> > >
> > > Hmm...
> > >
> >
> > It seems class deletes are buggy.
> >
> > After one "tc class del dev $ETH classid 11:1 ..."
> >
> > a "tc -s -d qdisc show dev $ETH" triggers an Oops
> >
>
> This seems to be due to:
>
> > +static void qfq_destroy_class(struct Qdisc *sch, struct qfq_class *cl)
> > +{
> > + struct qfq_sched *q = (struct qfq_sched *)sch;
>
> it should be:
>
> struct qfq_sched *q = sched_priv(sch);
>
> The same bug you identified in qfq_reset_qdisc() is present also in
> qfq_drop(), both loops need to be corrected...
>
> It should also be noted that this scheduler (like HFSC, IIRC) depends
> on the child qdisc not to reorder packets for the guarantees to be met,
> as the timestamps need to be in sync with the length of the packet at the
> head of the queue. If this can't be guaranteed, to preserve the formal
> correctness it should be changed to always use the maximum packet size
> to calculate the timestamps.
>
> @Stephen: not that I'm proud of that, but all the bugs found so far are mine...
I am going to test an updated version, thanks for all these hints !
^ permalink raw reply
* Re: [patch net-next-2.6] bonding: remove skb_share_check in handle_frame
From: Nicolas de Pesloüan @ 2011-03-03 8:37 UTC (permalink / raw)
To: Jiri Pirko; +Cc: Andy Gospodarek, netdev, davem, fubar, eric.dumazet
In-Reply-To: <20110303061433.GA2835@psychotron.redhat.com>
Le 03/03/2011 07:14, Jiri Pirko a écrit :
> Wed, Mar 02, 2011 at 10:54:03PM CET, nicolas.2p.debian@gmail.com wrote:
[snip]
>> All those handlers that call netif_rx() or __netif_receive_skb()
>> sound horrible to me. Can you imagine the global overhead of the
>> above receive path?
>>
>> The reason why I suggested you introduce the goto another_round is
>> because most - if not all - stacking configurations could/should be
>> handled simply by returning the right skb from the rx_handler and let
>> __netif_receive_skb() loop. And by having the right orig_dev logic
>> inside __netif_receive_skb(), it could be possible to remove the
>> current vlan_on_bond_hook hack.
>
> Well that wouldn't solve the problem. if we just got anorther_round the
> packed would not be delivered to bond0.100 but only to bond0. That's
> also unwanted. Well, this think shouldn't have been added here in the
> first place :(
Yes, the packet would be delivered "exact match" to bond0, and this is what the bonding ARP handler
expects, because it registered at this level, then deliver "exact match or NULL" to bond0.100, if
appropriate, to whatever other protocol handlers.
There is a subtile difference between ptype_all and ptype_base delivery:
- The ptype_all handlers will receive the frame between two call to rx_handlers.
- The ptype_base handlers will receive the frame after all rx_handlers (and hard coded handlers like
vlan) were called.
So a ptype_all handler might receive vlan tagged frame, if it register below the interface that
untag the frame, while a ptype_base handler will always receive the untagged frame, because
ptype_base delivery is done at the end of __netif_receive_skb().
I don't know whether it is desirable, but it used to be required for the bonding ARP monitor to work.
May be we should add some extra properties to protocol handler, so they can tell
__netif_receive_skb() what they expect :
- a property to tell whether the protocol handler want the exact frame that cross the interface they
registered on or the final frame, after all rx_handlers.
- a property to tell what they expect a the orig_dev value : the lowest interface known to
__netif_receive_skb() (f_packet) or the interface one level below the interface they registered on
(bonding ARP monitor).
Having those properties, I consider we would be in a position to remove most special hacks we
currently have to handle particular stacking configuration.
Nicolas.
^ permalink raw reply
* Re: [PATCH (sh-2.6) 1/4] clksource: Generic timer infrastructure
From: Arnd Bergmann @ 2011-03-03 8:45 UTC (permalink / raw)
To: Peppe CAVALLARO
Cc: Thomas Gleixner, Stuart MENEFY, linux-sh@vger.kernel.org,
netdev@vger.kernel.org, John Stultz, linux-kernel@vger.kernel.org,
linux@arm.linux.org.uk
In-Reply-To: <4D6E7FD7.1060408@st.com>
On Wednesday 02 March 2011, Peppe CAVALLARO wrote:
> At any rate, I am happy to use the stmmac as experimental
> driver to do this kind tests.
> Indeed, in the past, on old Kernel (IIRC 2.6.23), I tried to use
> the kernel timers but I removed the code from it because
> I had noticed packets loss and a strange phenomenon with cyclesoak
> (that showed broken sysload % during the heavy network activities).
>
> Let me know how to proceed:
>
> 1) experiment with stmmac and hrtimer for handling rx/tx?
> 2) rework the patches for the Generic Timer Infra?
I'd suggest doing the first. I'm surprised that using an unrelated
timer for processing interrupts even helps you on stmmac.
The timers that you'd normally use for rx interrupt mitigation
are not periodic timers but are started when a packet arrives
from the outside.
Doing periodic wakeups for RX instead of just waiting for
packets to come in should have a significant impact on power
management on an otherwise idle system.
For tx resource reclaim, a relatively slow oneshot timer (not
even hrtimer) should be good enough, since it only needs to be
active when there is no other way to clean up. E.g. when you
are in napi polling mode (interrupt disabled), you know that
stmmac_poll gets called soon, and you can also do the reclaim
from stmmac_xmit() in order to prevent the timer from triggering
when you are constantly transmitting.
Arnd
^ permalink raw reply
* [PATCH] bonding 802.3ad: Fix the state machine locking
From: Nils Carlson @ 2011-03-03 9:05 UTC (permalink / raw)
To: bonding-devel, netdev; +Cc: Nils Carlson
The current implementation only locked around the rx state machine,
but the rx state machine reads data touched by other state machines
as well, so in a very ugly scenario we would see the rx state machine
reading bad values as data was being overwritten by other state
machines. This patch moves the bond_3ad locking to protect all the
state machines from concurrency issues.
Signed-off-by: Nils Carlson <nils.carlson@ericsson.com>
---
drivers/net/bonding/bond_3ad.c | 32 +++++++++++++++++++-------------
drivers/net/bonding/bond_3ad.h | 2 +-
2 files changed, 20 insertions(+), 14 deletions(-)
diff --git a/drivers/net/bonding/bond_3ad.c b/drivers/net/bonding/bond_3ad.c
index 1024ae1..bbc473b 100644
--- a/drivers/net/bonding/bond_3ad.c
+++ b/drivers/net/bonding/bond_3ad.c
@@ -281,23 +281,23 @@ static inline int __check_agg_selection_timer(struct port *port)
}
/**
- * __get_rx_machine_lock - lock the port's RX machine
+ * __get_state_machine_lock - lock the port's state machine
* @port: the port we're looking at
*
*/
-static inline void __get_rx_machine_lock(struct port *port)
+static inline void __get_state_machine_lock(struct port *port)
{
- spin_lock_bh(&(SLAVE_AD_INFO(port->slave).rx_machine_lock));
+ spin_lock_bh(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
}
/**
- * __release_rx_machine_lock - unlock the port's RX machine
+ * __release_state_machine_lock - unlock the port's state machine
* @port: the port we're looking at
*
*/
-static inline void __release_rx_machine_lock(struct port *port)
+static inline void __release_state_machine_lock(struct port *port)
{
- spin_unlock_bh(&(SLAVE_AD_INFO(port->slave).rx_machine_lock));
+ spin_unlock_bh(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
}
/**
@@ -388,14 +388,14 @@ static u8 __get_duplex(struct port *port)
}
/**
- * __initialize_port_locks - initialize a port's RX machine spinlock
+ * __initialize_port_locks - initialize a port's state machine spinlock
* @port: the port we're looking at
*
*/
static inline void __initialize_port_locks(struct port *port)
{
// make sure it isn't called twice
- spin_lock_init(&(SLAVE_AD_INFO(port->slave).rx_machine_lock));
+ spin_lock_init(&(SLAVE_AD_INFO(port->slave).state_machine_lock));
}
//conversions
@@ -1025,9 +1025,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
{
rx_states_t last_state;
- // Lock to prevent 2 instances of this function to run simultaneously(rx interrupt and periodic machine callback)
- __get_rx_machine_lock(port);
-
// keep current State Machine state to compare later if it was changed
last_state = port->sm_rx_state;
@@ -1133,7 +1130,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
pr_err("%s: An illegal loopback occurred on adapter (%s).\n"
"Check the configuration to verify that all adapters are connected to 802.3ad compliant switch ports\n",
port->slave->dev->master->name, port->slave->dev->name);
- __release_rx_machine_lock(port);
return;
}
__update_selected(lacpdu, port);
@@ -1153,7 +1149,6 @@ static void ad_rx_machine(struct lacpdu *lacpdu, struct port *port)
break;
}
}
- __release_rx_machine_lock(port);
}
/**
@@ -2155,6 +2150,12 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
goto re_arm;
}
+ /* Lock around all the state machines to protect data that may
+ * be accessed also from the ad_rx_machine running in the
+ * interrupt handler.
+ */
+ __get_state_machine_lock(port);
+
ad_rx_machine(NULL, port);
ad_periodic_machine(port);
ad_port_selection_logic(port);
@@ -2164,6 +2165,8 @@ void bond_3ad_state_machine_handler(struct work_struct *work)
// turn off the BEGIN bit, since we already handled it
if (port->sm_vars & AD_PORT_BEGIN)
port->sm_vars &= ~AD_PORT_BEGIN;
+
+ __release_state_machine_lock(port);
}
re_arm:
@@ -2200,7 +2203,10 @@ static void bond_3ad_rx_indication(struct lacpdu *lacpdu, struct slave *slave, u
case AD_TYPE_LACPDU:
pr_debug("Received LACPDU on port %d\n",
port->actor_port_number);
+ /* Protect against concurrent state machines */
+ __get_state_machine_lock(port);
ad_rx_machine(lacpdu, port);
+ __release_state_machine_lock(port);
break;
case AD_TYPE_MARKER:
diff --git a/drivers/net/bonding/bond_3ad.h b/drivers/net/bonding/bond_3ad.h
index 2c46a15..9182506 100644
--- a/drivers/net/bonding/bond_3ad.h
+++ b/drivers/net/bonding/bond_3ad.h
@@ -264,7 +264,7 @@ struct ad_bond_info {
struct ad_slave_info {
struct aggregator aggregator; // 802.3ad aggregator structure
struct port port; // 802.3ad port structure
- spinlock_t rx_machine_lock; // To avoid race condition between callback and receive interrupt
+ spinlock_t state_machine_lock; // To avoid race condition between callback and receive interrupt
u16 id;
};
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Eric Dumazet @ 2011-03-03 9:07 UTC (permalink / raw)
To: Fabio Checconi
Cc: Stephen Hemminger, David Miller, Luigi Rizzo, netdev,
Paolo Valente
In-Reply-To: <20110303081940.GA29685@gandalf.sssup.it>
Le jeudi 03 mars 2011 à 09:19 +0100, Fabio Checconi a écrit :
> The same bug you identified in qfq_reset_qdisc() is present also in
> qfq_drop(), both loops need to be corrected...
I dont think so, because in qfq_drop() we exit from qfq_drop() right
after call to qfq_deactivate_class()
But I agree code should be same ;)
^ permalink raw reply
* [PATCH] sctp: fix the fast retransmit limit
From: Wei Yongjun @ 2011-03-03 9:29 UTC (permalink / raw)
To: David Miller, Vlad Yasevich, netdev@vger.kernel.org, lksctp
Cc: Mingyuan Zhu, Neil Horman
If chunk is still lost after fast retransmit, SCTP stack will
never allow the second fast retransmit of this chunk, even if
the peer need we do this. This chunk will be retransmit until
the rtx timeout. This limit is introduce by the following patch:
sctp: reduce memory footprint of sctp_chunk structure
(c226ef9b83694311327f3ab0036c6de9c22e9daf)
This patch revert this limit and removed useless SCTP_DONT_FRTX.
Signed-off-by: Wei Yongjun <yjwei@cn.fujitsu.com>
---
include/net/sctp/structs.h | 1 -
net/sctp/outqueue.c | 4 ++--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index cc9185c..82a0f84 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -751,7 +751,6 @@ struct sctp_chunk {
#define SCTP_CAN_FRTX 0x0
#define SCTP_NEED_FRTX 0x1
-#define SCTP_DONT_FRTX 0x2
__u16 rtt_in_progress:1, /* This chunk used for RTT calc? */
has_tsn:1, /* Does this chunk have a TSN yet? */
has_ssn:1, /* Does this chunk have a SSN yet? */
diff --git a/net/sctp/outqueue.c b/net/sctp/outqueue.c
index 8c6d379..7ed5862 100644
--- a/net/sctp/outqueue.c
+++ b/net/sctp/outqueue.c
@@ -657,7 +657,7 @@ redo:
* after it is retransmitted.
*/
if (chunk->fast_retransmit == SCTP_NEED_FRTX)
- chunk->fast_retransmit = SCTP_DONT_FRTX;
+ chunk->fast_retransmit = SCTP_CAN_FRTX;
q->empty = 0;
break;
@@ -679,7 +679,7 @@ redo:
if (rtx_timeout || fast_rtx) {
list_for_each_entry(chunk1, lqueue, transmitted_list) {
if (chunk1->fast_retransmit == SCTP_NEED_FRTX)
- chunk1->fast_retransmit = SCTP_DONT_FRTX;
+ chunk1->fast_retransmit = SCTP_CAN_FRTX;
}
}
--
1.6.5.2
^ permalink raw reply related
* Re: RFC v1: sysctl: add sysctl header cookie, share tables between nets
From: Eric W. Biederman @ 2011-03-03 9:33 UTC (permalink / raw)
To: David Miller; +Cc: lucian.grijincu, adobriyan, tavi, netdev, linux-kernel
In-Reply-To: <20110302.170644.58413369.davem@davemloft.net>
David Miller <davem@davemloft.net> writes:
> From: Lucian Adrian Grijincu <lucian.grijincu@gmail.com>
> Date: Fri, 25 Feb 2011 20:52:32 +0200
>
>> This is a new approach to the "share sysctl tables" RFC series I
>> posted earlier this month.
>
> I do not disagree conceptually with these changes from a networking
> perspective, but I am not a sysctl layer expert so I don't know if the
> generic sysctl bits are a good idea or not.
I may be missing something in these patches. I haven't had time to look
at this most recent batch carefully. But from a 10,000 foot perspective I
have a problem with them. With a handful of network devices the size of
the data structures is negligible.
Where problems show up is when you have a lot of sysctl entries for
devices and at that point we have much larger problems using the
sysctl data structures. Today add/remove are big O(previous entries)
and I think even readdir suffers from non-scalable data structures.
There are other related issues that the sysctl data structures are not
optimized for use in /proc, and that sysctl uses so usable but on off
locking like mechanisms.
Changing things to make the sysctl users more dependent on the current
implement details of the sysctl data structures seems the exact
opposite of the direction we need to go to make the sysctl data structures
scale.
So until I can see a reason why we should save a few bytes at the cost
of greater future maintenance costs I'm not in favor of this patch set.
Eric
^ permalink raw reply
* Re: [PATCH] sched: QFQ - quick fair queue scheduler (v2)
From: Eric Dumazet @ 2011-03-03 9:35 UTC (permalink / raw)
To: Patrick McHardy
Cc: Stephen Hemminger, Fabio Checconi, David Miller, Luigi Rizzo,
netdev
In-Reply-To: <4D6E8997.4010001@trash.net>
Le mercredi 02 mars 2011 à 19:16 +0100, Patrick McHardy a écrit :
> Am 02.03.2011 18:31, schrieb Eric Dumazet:
> > Le mercredi 02 mars 2011 à 17:18 +0100, Eric Dumazet a écrit :
> >> Le mercredi 02 mars 2011 à 08:11 -0800, Stephen Hemminger a écrit :
> >>
> >>> I put the iproute2 code into the repository in the experimental branch.
> >>>
> >>
> >> Thanks
> >>
> >> It seems as soon as packets are dropped, qdisc is frozen (no more
> >> packets dequeued)
> >>
> >> Hmm...
> >>
> >
> > It seems class deletes are buggy.
> >
> > After one "tc class del dev $ETH classid 11:1 ..."
> >
> > a "tc -s -d qdisc show dev $ETH" triggers an Oops
>
> What classes and filters did you have before issuing that command?
(Fabio found the origin of the problem in qfq_destroy_class())
Here is the script I wrote to use QFQ on dummy0 device
(I then use a normal udpflood workload to stress the thing)
More or less copied from other stuff I had, so its probably very
stupid ;)
# cat egress_dummy0.sh
ETH=dummy0
RATE="rate 40Mbit"
PRIVNETS="172.16.0.0/12 192.168.0.0/16 10.0.0.0/8"
ALLOT="allot 20000"
ethtool -K $ETH tso off
tc qdisc del dev $ETH root 2>/dev/null
tc qdisc add dev $ETH root handle 1: cbq avpkt 1000 rate 1000Mbit \
bandwidth 1000Mbit
tc class add dev $ETH parent 1: classid 1:1 \
est 1sec 8sec cbq allot 10000 mpu 64 \
rate 1000Mbit prio 1 avpkt 1500 bounded
# output to private nets : 40 Mbit limit
tc class add dev $ETH parent 1:1 classid 1:11 \
est 1sec 8sec cbq $ALLOT mpu 64 \
$RATE prio 2 avpkt 1400 bounded
tc qdisc add dev $ETH parent 1:11 handle 11: \
est 1sec 8sec qfq
tc filter add dev $ETH protocol ip parent 11: handle 3 \
flow hash keys rxhash divisor 16
tc class add dev $ETH classid 11:1 est 0.5sec 2sec qfq weight 100 maxpkt 1100
tc class add dev $ETH classid 11:2 est 0.5sec 2sec qfq weight 200 maxpkt 1200
tc class add dev $ETH classid 11:3 est 0.5sec 2sec qfq weight 300 maxpkt 1300
tc class add dev $ETH classid 11:4 est 0.5sec 2sec qfq weight 400 maxpkt 1400
tc class add dev $ETH classid 11:5 est 0.5sec 2sec qfq weight 500 maxpkt 1500
tc class add dev $ETH classid 11:6 est 0.5sec 2sec qfq weight 600 maxpkt 1600
tc class add dev $ETH classid 11:7 est 0.5sec 2sec qfq weight 700 maxpkt 1700
tc class add dev $ETH classid 11:8 est 0.5sec 2sec qfq weight 800 maxpkt 1800
tc class add dev $ETH classid 11:9 est 0.5sec 2sec qfq weight 900 maxpkt 1900
tc class add dev $ETH classid 11:a est 0.5sec 2sec qfq weight 1000 maxpkt 2000
tc class add dev $ETH classid 11:b est 0.5sec 2sec qfq weight 1100 maxpkt 2048
tc class add dev $ETH classid 11:c est 0.5sec 2sec qfq weight 1200 maxpkt 2048
tc class add dev $ETH classid 11:d est 0.5sec 2sec qfq weight 1300 maxpkt 2048
tc class add dev $ETH classid 11:e est 0.5sec 2sec qfq weight 1400 maxpkt 2048
tc class add dev $ETH classid 11:f est 0.5sec 2sec qfq weight 1500 maxpkt 2048
tc class add dev $ETH classid 11:10 est 0.5sec 2sec qfq weight 1600 maxpkt 2048
for privnet in $PRIVNETS
do
tc filter add dev $ETH parent 1: protocol ip prio 100 u32 \
match ip dst $privnet flowid 1:11
done
tc filter add dev $ETH parent 1: protocol ip prio 100 u32 \
match ip protocol 0 0x00 flowid 1:1
^ permalink raw reply
* [PATCH 1/2] netlink: kill loginuid/sessionid/sid members from struct netlink_skb_parms
From: Patrick McHardy @ 2011-03-03 9:38 UTC (permalink / raw)
To: NetDev; +Cc: David S. Miller, Al Viro, Eric Paris, linux-audit
[-- Attachment #1: Type: text/plain, Size: 148 bytes --]
The following two patches kill a few unnecessary members of the
netlink cb in order to make room for new members needed for memory
mapped netlink.
[-- Attachment #2: 01.diff --]
[-- Type: text/plain, Size: 9647 bytes --]
commit 509c35212e0d2362393597f195dc5da3fafd4a9c
Author: Patrick McHardy <kaber@trash.net>
Date: Thu Mar 3 10:17:28 2011 +0100
netlink: kill loginuid/sessionid/sid members from struct netlink_skb_parms
Netlink message processing in the kernel is synchronous these days, the
session information can be collected when needed.
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index e2b9e63..66823b8 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -161,9 +161,6 @@ struct netlink_skb_parms {
__u32 pid;
__u32 dst_group;
kernel_cap_t eff_cap;
- __u32 loginuid; /* Login (audit) uid */
- __u32 sessionid; /* Session id (audit) */
- __u32 sid; /* SELinux security id */
};
#define NETLINK_CB(skb) (*(struct netlink_skb_parms*)&((skb)->cb))
diff --git a/kernel/audit.c b/kernel/audit.c
index 162e88e..9395003 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -673,9 +673,9 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
pid = NETLINK_CREDS(skb)->pid;
uid = NETLINK_CREDS(skb)->uid;
- loginuid = NETLINK_CB(skb).loginuid;
- sessionid = NETLINK_CB(skb).sessionid;
- sid = NETLINK_CB(skb).sid;
+ loginuid = audit_get_loginuid(current);
+ sessionid = audit_get_sessionid(current);
+ security_task_getsecid(current, &sid);
seq = nlh->nlmsg_seq;
data = NLMSG_DATA(nlh);
diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
index add2819..f8277c8 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1238,6 +1238,7 @@ static int audit_filter_user_rules(struct netlink_skb_parms *cb,
for (i = 0; i < rule->field_count; i++) {
struct audit_field *f = &rule->fields[i];
int result = 0;
+ u32 sid;
switch (f->type) {
case AUDIT_PID:
@@ -1250,19 +1251,22 @@ static int audit_filter_user_rules(struct netlink_skb_parms *cb,
result = audit_comparator(cb->creds.gid, f->op, f->val);
break;
case AUDIT_LOGINUID:
- result = audit_comparator(cb->loginuid, f->op, f->val);
+ result = audit_comparator(audit_get_loginuid(current),
+ f->op, f->val);
break;
case AUDIT_SUBJ_USER:
case AUDIT_SUBJ_ROLE:
case AUDIT_SUBJ_TYPE:
case AUDIT_SUBJ_SEN:
case AUDIT_SUBJ_CLR:
- if (f->lsm_rule)
- result = security_audit_rule_match(cb->sid,
+ if (f->lsm_rule) {
+ security_task_getsecid(current, &sid);
+ result = security_audit_rule_match(sid,
f->type,
f->op,
f->lsm_rule,
NULL);
+ }
break;
}
diff --git a/net/netlabel/netlabel_user.h b/net/netlabel/netlabel_user.h
index 6caef8b..f4fc4c9 100644
--- a/net/netlabel/netlabel_user.h
+++ b/net/netlabel/netlabel_user.h
@@ -49,9 +49,9 @@
static inline void netlbl_netlink_auditinfo(struct sk_buff *skb,
struct netlbl_audit *audit_info)
{
- audit_info->secid = NETLINK_CB(skb).sid;
- audit_info->loginuid = NETLINK_CB(skb).loginuid;
- audit_info->sessionid = NETLINK_CB(skb).sessionid;
+ security_task_getsecid(current, &audit_info->secid);
+ audit_info->loginuid = audit_get_loginuid(current);
+ audit_info->sessionid = audit_get_sessionid(current);
}
/* NetLabel NETLINK I/O functions */
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 478181d..97ecd92 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1362,9 +1362,6 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
NETLINK_CB(skb).pid = nlk->pid;
NETLINK_CB(skb).dst_group = dst_group;
- NETLINK_CB(skb).loginuid = audit_get_loginuid(current);
- NETLINK_CB(skb).sessionid = audit_get_sessionid(current);
- security_task_getsecid(current, &(NETLINK_CB(skb).sid));
memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
/* What can I do? Netlink is asynchronous, so that
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 673698d..468ab60 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -497,9 +497,9 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
struct xfrm_state *x;
int err;
struct km_event c;
- uid_t loginuid = NETLINK_CB(skb).loginuid;
- u32 sessionid = NETLINK_CB(skb).sessionid;
- u32 sid = NETLINK_CB(skb).sid;
+ uid_t loginuid = audit_get_loginuid(current);
+ u32 sessionid = audit_get_sessionid(current);
+ u32 sid;
err = verify_newsa_info(p, attrs);
if (err)
@@ -515,6 +515,7 @@ static int xfrm_add_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
else
err = xfrm_state_update(x);
+ security_task_getsecid(current, &sid);
xfrm_audit_state_add(x, err ? 0 : 1, loginuid, sessionid, sid);
if (err < 0) {
@@ -575,9 +576,9 @@ static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
int err = -ESRCH;
struct km_event c;
struct xfrm_usersa_id *p = nlmsg_data(nlh);
- uid_t loginuid = NETLINK_CB(skb).loginuid;
- u32 sessionid = NETLINK_CB(skb).sessionid;
- u32 sid = NETLINK_CB(skb).sid;
+ uid_t loginuid = audit_get_loginuid(current);
+ u32 sessionid = audit_get_sessionid(current);
+ u32 sid;
x = xfrm_user_state_lookup(net, p, attrs, &err);
if (x == NULL)
@@ -602,6 +603,7 @@ static int xfrm_del_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
km_state_notify(x, &c);
out:
+ security_task_getsecid(current, &sid);
xfrm_audit_state_delete(x, err ? 0 : 1, loginuid, sessionid, sid);
xfrm_state_put(x);
return err;
@@ -1265,9 +1267,9 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
struct km_event c;
int err;
int excl;
- uid_t loginuid = NETLINK_CB(skb).loginuid;
- u32 sessionid = NETLINK_CB(skb).sessionid;
- u32 sid = NETLINK_CB(skb).sid;
+ uid_t loginuid = audit_get_loginuid(current);
+ u32 sessionid = audit_get_sessionid(current);
+ u32 sid;
err = verify_newpolicy_info(p);
if (err)
@@ -1286,6 +1288,7 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
* a type XFRM_MSG_UPDPOLICY - JHS */
excl = nlh->nlmsg_type == XFRM_MSG_NEWPOLICY;
err = xfrm_policy_insert(p->dir, xp, excl);
+ security_task_getsecid(current, &sid);
xfrm_audit_policy_add(xp, err ? 0 : 1, loginuid, sessionid, sid);
if (err) {
@@ -1522,10 +1525,11 @@ static int xfrm_get_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
NETLINK_CB(skb).pid);
}
} else {
- uid_t loginuid = NETLINK_CB(skb).loginuid;
- u32 sessionid = NETLINK_CB(skb).sessionid;
- u32 sid = NETLINK_CB(skb).sid;
+ uid_t loginuid = audit_get_loginuid(current);
+ u32 sessionid = audit_get_sessionid(current);
+ u32 sid;
+ security_task_getsecid(current, &sid);
xfrm_audit_policy_delete(xp, err ? 0 : 1, loginuid, sessionid,
sid);
@@ -1553,9 +1557,9 @@ static int xfrm_flush_sa(struct sk_buff *skb, struct nlmsghdr *nlh,
struct xfrm_audit audit_info;
int err;
- audit_info.loginuid = NETLINK_CB(skb).loginuid;
- audit_info.sessionid = NETLINK_CB(skb).sessionid;
- audit_info.secid = NETLINK_CB(skb).sid;
+ audit_info.loginuid = audit_get_loginuid(current);
+ audit_info.sessionid = audit_get_sessionid(current);
+ security_task_getsecid(current, &audit_info.secid);
err = xfrm_state_flush(net, p->proto, &audit_info);
if (err) {
if (err == -ESRCH) /* empty table */
@@ -1720,9 +1724,9 @@ static int xfrm_flush_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
if (err)
return err;
- audit_info.loginuid = NETLINK_CB(skb).loginuid;
- audit_info.sessionid = NETLINK_CB(skb).sessionid;
- audit_info.secid = NETLINK_CB(skb).sid;
+ audit_info.loginuid = audit_get_loginuid(current);
+ audit_info.sessionid = audit_get_sessionid(current);
+ security_task_getsecid(current, &audit_info.secid);
err = xfrm_policy_flush(net, type, &audit_info);
if (err) {
if (err == -ESRCH) /* empty table */
@@ -1789,9 +1793,11 @@ static int xfrm_add_pol_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
err = 0;
if (up->hard) {
- uid_t loginuid = NETLINK_CB(skb).loginuid;
- uid_t sessionid = NETLINK_CB(skb).sessionid;
- u32 sid = NETLINK_CB(skb).sid;
+ uid_t loginuid = audit_get_loginuid(current);
+ u32 sessionid = audit_get_sessionid(current);
+ u32 sid;
+
+ security_task_getsecid(current, &sid);
xfrm_policy_delete(xp, p->dir);
xfrm_audit_policy_delete(xp, 1, loginuid, sessionid, sid);
@@ -1830,9 +1836,11 @@ static int xfrm_add_sa_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
km_state_expired(x, ue->hard, current->pid);
if (ue->hard) {
- uid_t loginuid = NETLINK_CB(skb).loginuid;
- uid_t sessionid = NETLINK_CB(skb).sessionid;
- u32 sid = NETLINK_CB(skb).sid;
+ uid_t loginuid = audit_get_loginuid(current);
+ u32 sessionid = audit_get_sessionid(current);
+ u32 sid;
+
+ security_task_getsecid(current, &sid);
__xfrm_state_delete(x);
xfrm_audit_state_delete(x, 1, loginuid, sessionid, sid);
}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index c8d6992..cef42f5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4669,6 +4669,7 @@ static int selinux_netlink_recv(struct sk_buff *skb, int capability)
{
int err;
struct common_audit_data ad;
+ u32 sid;
err = cap_netlink_recv(skb, capability);
if (err)
@@ -4677,8 +4678,9 @@ static int selinux_netlink_recv(struct sk_buff *skb, int capability)
COMMON_AUDIT_DATA_INIT(&ad, CAP);
ad.u.cap = capability;
- return avc_has_perm(NETLINK_CB(skb).sid, NETLINK_CB(skb).sid,
- SECCLASS_CAPABILITY, CAP_TO_MASK(capability), &ad);
+ security_task_getsecid(current, &sid);
+ return avc_has_perm(sid, sid, SECCLASS_CAPABILITY,
+ CAP_TO_MASK(capability), &ad);
}
static int ipc_alloc_security(struct task_struct *task,
^ permalink raw reply related
* [PATCH 2/2] netlink: kill eff_cap from struct netlink_skb_parms
From: Patrick McHardy @ 2011-03-03 9:38 UTC (permalink / raw)
To: NetDev, dm-devel
Cc: David S. Miller, Chris Wright,
linux-security-module@vger.kernel.org, drbd-dev
[-- Attachment #1: Type: text/plain, Size: 0 bytes --]
[-- Attachment #2: 02.diff --]
[-- Type: text/plain, Size: 2971 bytes --]
commit 8ff259625f0ab295fa085b0718eed13093813fbc
Author: Patrick McHardy <kaber@trash.net>
Date: Thu Mar 3 10:17:31 2011 +0100
netlink: kill eff_cap from struct netlink_skb_parms
Netlink message processing in the kernel is synchronous these days,
capabilities can be checked directly in security_netlink_recv() from
the current process.
Signed-off-by: Patrick McHardy <kaber@trash.net>
diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
index 8cbfaa6..fe81c85 100644
--- a/drivers/block/drbd/drbd_nl.c
+++ b/drivers/block/drbd/drbd_nl.c
@@ -2177,7 +2177,7 @@ static void drbd_connector_callback(struct cn_msg *req, struct netlink_skb_parms
return;
}
- if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN)) {
+ if (!cap_raised(current_cap(), CAP_SYS_ADMIN)) {
retcode = ERR_PERM;
goto fail;
}
diff --git a/drivers/md/dm-log-userspace-transfer.c b/drivers/md/dm-log-userspace-transfer.c
index 049eaf1..1f23e04 100644
--- a/drivers/md/dm-log-userspace-transfer.c
+++ b/drivers/md/dm-log-userspace-transfer.c
@@ -134,7 +134,7 @@ static void cn_ulog_callback(struct cn_msg *msg, struct netlink_skb_parms *nsp)
{
struct dm_ulog_request *tfr = (struct dm_ulog_request *)(msg + 1);
- if (!cap_raised(nsp->eff_cap, CAP_SYS_ADMIN))
+ if (!cap_raised(current_cap(), CAP_SYS_ADMIN))
return;
spin_lock(&receiving_list_lock);
diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 66823b8..4c4ac3f 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -160,7 +160,6 @@ struct netlink_skb_parms {
struct ucred creds; /* Skb credentials */
__u32 pid;
__u32 dst_group;
- kernel_cap_t eff_cap;
};
#define NETLINK_CB(skb) (*(struct netlink_skb_parms*)&((skb)->cb))
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 97ecd92..a808fb1 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1364,12 +1364,6 @@ static int netlink_sendmsg(struct kiocb *kiocb, struct socket *sock,
NETLINK_CB(skb).dst_group = dst_group;
memcpy(NETLINK_CREDS(skb), &siocb->scm->creds, sizeof(struct ucred));
- /* What can I do? Netlink is asynchronous, so that
- we will have to save current capabilities to
- check them, when this message will be delivered
- to corresponding kernel module. --ANK (980802)
- */
-
err = -EFAULT;
if (memcpy_fromiovec(skb_put(skb, len), msg->msg_iov, len)) {
kfree_skb(skb);
diff --git a/security/commoncap.c b/security/commoncap.c
index 64c2ed9..a83e607 100644
--- a/security/commoncap.c
+++ b/security/commoncap.c
@@ -52,13 +52,12 @@ static void warn_setuid_and_fcaps_mixed(const char *fname)
int cap_netlink_send(struct sock *sk, struct sk_buff *skb)
{
- NETLINK_CB(skb).eff_cap = current_cap();
return 0;
}
int cap_netlink_recv(struct sk_buff *skb, int cap)
{
- if (!cap_raised(NETLINK_CB(skb).eff_cap, cap))
+ if (!cap_raised(current_cap(), cap))
return -EPERM;
return 0;
}
^ permalink raw reply related
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