revert: net: Make skb->skb_iif always track skb->dev

RFC In review Oliver Hartkopp 7 participants 24 messages Jan 12, 2013 – Jan 14, 2013

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Oliver Hartkopp

author

2013-01-12 13:48 UTC #0 lore raw

RFC 1/1 revert: net: Make skb->skb_iif always track skb->dev

Hello Dave,

in your below patch from 23 Jul 2012 you removed the check for an already set value of skb_iif in net/core/dev.c

I'm currently working on a solution to prevent some routed CAN frames to be sent back onto the originating network device.

With your patch it is not possible anymore to check on which netdev the CAN frame has originally been received, as for every routing the frame goes through netif_receive_skb(), which hard sets

	skb->skb_iif = skb->dev->ifindex

and therefore kills the original incoming interface index.

To me it is not clear why skb_iff is needed anyway as the value should always be available via skb->dev->ifindex, right?

But if skb_iff has any right to exist it should contain the first incoming interface on the host IMO.

Please correct my if i'm wrong and/or tell me what your commit message means in respect to my request and why skb->dev->ifindex is not used instead of skb_iif. I feel somehow lost about the skb_iif intention ...

Best regards,
Oliver

---

http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commitdiff;h=b68581778cd0051a3fb9a2b614dee7eccb5127ff

net: Make skb->skb_iif always track skb->dev

Make it follow device decapsulation, from things such as VLAN and bonding.

The stuff that actually cares about pre-demuxed device pointers, is handled by the "orig_dev" variable in __netif_receive_skb(). And the only consumer of that is the po->origdev feature of AF_PACKET sockets.

Signed-off-by David S. Miller <[email protected]>

---

net/core/dev.c +1 -2

index cca02ae..0ebaea1 100644
@@ -3173,8 +3173,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	if (netpoll_receive_skb(skb))
 		return NET_RX_DROP;
 
-	if (!skb->skb_iif)
-		skb->skb_iif = skb->dev->ifindex;
 	orig_dev = skb->dev;
 
 	skb_reset_network_header(skb);

@@ -3186,6 +3184,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	rcu_read_lock();
 
 another_round:
+	skb->skb_iif = skb->dev->ifindex;
 
 	__this_cpu_inc(softnet_data.processed);

Jiri Pirko

maintainer

2013-01-12 18:13 UTC #1 lore raw

Quoted 7 lines · Sat, Jan 12, 2013 at 02:48:14PM CET, [email protected] wrote:

Hello Dave,
in your below patch from 23 Jul 2012 you removed the check for an already set value of skb_iif in net/core/dev.c
I'm currently working on a solution to prevent some routed CAN frames to be sent back onto the originating network device.

Hm, I'm not sure where exactly you want to use this information, but can_rcv() can get it from orig_dev->ifindex

Quoted 12 lines

With your patch it is not possible anymore to check on which netdev the CAN frame has originally been received, as for every routing the frame goes through netif_receive_skb(), which hard sets
	skb->skb_iif = skb->dev->ifindex
and therefore kills the original incoming interface index.
To me it is not clear why skb_iff is needed anyway as the value should always be available via skb->dev->ifindex, right?

Quoted 6 lines

But if skb_iff has any right to exist it should contain the first incoming interface on the host IMO.
Please correct my if i'm wrong and/or tell me what your commit message means in respect to my request and why skb->dev->ifindex is not used instead of skb_iif. I feel somehow lost about the skb_iif intention ...

I believe that skb_iif should be removed.

Quoted 49 lines

Best regards,
Oliver
---
http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commitdiff;h=b68581778cd0051a3fb9a2b614dee7eccb5127ff
net: Make skb->skb_iif always track skb->dev
Make it follow device decapsulation, from things such as VLAN and bonding.
The stuff that actually cares about pre-demuxed device pointers, is handled by the "orig_dev" variable in __netif_receive_skb(). And the only consumer of that is the po->origdev feature of AF_PACKET sockets.
Signed-off-by David S. Miller <[email protected]>
---
net/core/dev.c +0 -2
index cca02ae..0ebaea1 100644
@@ -3173,8 +3173,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	if (netpoll_receive_skb(skb))
 		return NET_RX_DROP;
 
-	if (!skb->skb_iif)
-		skb->skb_iif = skb->dev->ifindex;
 	orig_dev = skb->dev;
 
 	skb_reset_network_header(skb);
@@ -3186,6 +3184,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	rcu_read_lock();
 
another_round:
net/core/dev.c +1 -0
+	skb->skb_iif = skb->dev->ifindex;
	__this_cpu_inc(softnet_data.processed);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Oliver Hartkopp

author

2013-01-12 18:40 UTC #2 lore raw

Hello Jiri,

Quoted 11 lines · Jiri Pirko

Quoted 7 lines · Sat, Jan 12, 2013 at 02:48:14PM CET, [email protected] wrote:
Hello Dave,
in your below patch from 23 Jul 2012 you removed the check for an already set value of skb_iif in net/core/dev.c
I'm currently working on a solution to prevent some routed CAN frames to be sent back onto the originating network device.
Hm, I'm not sure where exactly you want to use this information, but can_rcv() can get it from orig_dev->ifindex

No - it's not in can_rcv() ...

Depending on the filter lists in can_rcv() the received skb is passed to the function can_can_gw_rcv() in net/can/gw.c.

An there i wanted to add this code to omit sending the CAN frame on the originating interface:

@@ .. @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data

        if (!(gwj->dst.dev->flags & IFF_UP)) {
                gwj->dropped_frames++;
                return;
        }

+ /* is sending the skb back to the incoming interface allowed? */
+ if (!(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK) &&
+ skb->skb_iif == gwj->dst.dev->ifindex)
+ return;
+

        /*
         * clone the given skb, which has not been done in can_rcv()
         *

This works fine, when the patch from Dave is reverted.

I did not find any good solution to preserve the originating netdev over several netif_receive_skb() calls - but this skb_iif which has been made unusable ...

(..)

Quoted 4 lines

Quoted 4 lines
To me it is not clear why skb_iff is needed anyway as the value should always be available via skb->dev->ifindex, right?

Quoted 8 lines

Quoted 6 lines
But if skb_iff has any right to exist it should contain the first incoming interface on the host IMO.
Please correct my if i'm wrong and/or tell me what your commit message means in respect to my request and why skb->dev->ifindex is not used instead of skb_iif. I feel somehow lost about the skb_iif intention ...
I believe that skb_iif should be removed.

AFAICS skb->skb_iif is used as some kind of cached variable for the original skb->dev->ifindex - or is it really possible that skb->skb_iif is set while skb->dev->ifindex is not accessible?

Btw. i my case skb_iif would make sense though.

Regards,
Oliver

Quoted 46 lines

Quoted 46 lines
---
http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commitdiff;h=b68581778cd0051a3fb9a2b614dee7eccb5127ff
net: Make skb->skb_iif always track skb->dev
Make it follow device decapsulation, from things such as VLAN and bonding.
The stuff that actually cares about pre-demuxed device pointers, is handled by the "orig_dev" variable in __netif_receive_skb(). And the only consumer of that is the po->origdev feature of AF_PACKET sockets.
Signed-off-by David S. Miller <[email protected]>
---
net/core/dev.c +0 -2
index cca02ae..0ebaea1 100644
@@ -3173,8 +3173,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	if (netpoll_receive_skb(skb))
 		return NET_RX_DROP;
 
-	if (!skb->skb_iif)
-		skb->skb_iif = skb->dev->ifindex;
 	orig_dev = skb->dev;
 
 	skb_reset_network_header(skb);
@@ -3186,6 +3184,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	rcu_read_lock();
 
another_round:
net/core/dev.c +1 -0
+	skb->skb_iif = skb->dev->ifindex;
	__this_cpu_inc(softnet_data.processed);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Jiri Pirko

maintainer

2013-01-12 19:37 UTC #3 lore raw

Quoted 41 lines · Sat, Jan 12, 2013 at 07:40:33PM CET, [email protected] wrote:

Hello Jiri,
Quoted 11 lines · Jiri Pirko
Quoted 7 lines · Sat, Jan 12, 2013 at 02:48:14PM CET, [email protected] wrote:
Hello Dave,
in your below patch from 23 Jul 2012 you removed the check for an already set value of skb_iif in net/core/dev.c
I'm currently working on a solution to prevent some routed CAN frames to be sent back onto the originating network device.
Hm, I'm not sure where exactly you want to use this information, but can_rcv() can get it from orig_dev->ifindex
No - it's not in can_rcv() ...
Depending on the filter lists in can_rcv() the received skb is passed to the function can_can_gw_rcv() in net/can/gw.c.
An there i wanted to add this code to omit sending the CAN frame on the originating interface:
@@ .. @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data
       if (!(gwj->dst.dev->flags & IFF_UP)) {
               gwj->dropped_frames++;
               return;
       }
(quoted diff) +5 -0
+       /* is sending the skb back to the incoming interface allowed? */
+       if (!(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK) &&
+           skb->skb_iif == gwj->dst.dev->ifindex)
+               return;
+
       /*
        * clone the given skb, which has not been done in can_rcv()
        *

Quoted 5 lines

This works fine, when the patch from Dave is reverted.
I did not find any good solution to preserve the originating netdev over several netif_receive_skb() calls - but this skb_iif which has been made unusable ...

Well, look at struct receiver in net/can/af_can.h:

struct receiver {
        struct hlist_node list;
        struct rcu_head rcu;
        canid_t can_id;
        canid_t mask;
        unsigned long matches;
        void (*func)(struct sk_buff *, void *);
        void *data;
        char *ident;
};

your can_can_gw_rcv() is callback registered as ->func here.
This ->func is called from chain can_rcv->can_receive->can_rcv_filter->deliver

So just extend
->func to something like:

        void (*func)(struct sk_buff *, struct neti_device *orig_dev, void *);

and pass the orig_dev all the way through the chain.

Quoted 78 lines

(..)
Quoted 4 lines
Quoted 4 lines
To me it is not clear why skb_iff is needed anyway as the value should always be available via skb->dev->ifindex, right?
Quoted 8 lines
Quoted 6 lines
But if skb_iff has any right to exist it should contain the first incoming interface on the host IMO.
Please correct my if i'm wrong and/or tell me what your commit message means in respect to my request and why skb->dev->ifindex is not used instead of skb_iif. I feel somehow lost about the skb_iif intention ...
I believe that skb_iif should be removed.
AFAICS skb->skb_iif is used as some kind of cached variable for the original skb->dev->ifindex - or is it really possible that skb->skb_iif is set while skb->dev->ifindex is not accessible?
Btw. i my case skb_iif would make sense though.
Regards,
Oliver
Quoted 38 lines
Quoted 38 lines
---
http://git.kernel.org/?p=linux/kernel/git/davem/net-next.git;a=commitdiff;h=b68581778cd0051a3fb9a2b614dee7eccb5127ff
net: Make skb->skb_iif always track skb->dev
Make it follow device decapsulation, from things such as VLAN and bonding.
The stuff that actually cares about pre-demuxed device pointers, is handled by the "orig_dev" variable in __netif_receive_skb(). And the only consumer of that is the po->origdev feature of AF_PACKET sockets.
Signed-off-by David S. Miller <[email protected]>
---
net/core/dev.c +0 -2
index cca02ae..0ebaea1 100644
@@ -3173,8 +3173,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	if (netpoll_receive_skb(skb))
 		return NET_RX_DROP;
 
-	if (!skb->skb_iif)
-		skb->skb_iif = skb->dev->ifindex;
 	orig_dev = skb->dev;
 
 	skb_reset_network_header(skb);
@@ -3186,6 +3184,7 @@ static int __netif_receive_skb(struct sk_buff *skb)
 	rcu_read_lock();
 
another_round:
net/core/dev.c +1 -0
+	skb->skb_iif = skb->dev->ifindex;
Quoted 8 lines
Quoted 8 lines
	__this_cpu_inc(softnet_data.processed);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Oliver Hartkopp

author

2013-01-12 20:14 UTC #4 lore raw

Quoted 43 lines · Jiri Pirko

Quoted 19 lines · Sat, Jan 12, 2013 at 07:40:33PM CET, [email protected] wrote:
An there i wanted to add this code to omit sending the CAN frame on the originating interface:
@@ .. @@ static void can_can_gw_rcv(struct sk_buff *skb, void *data
       if (!(gwj->dst.dev->flags & IFF_UP)) {
               gwj->dropped_frames++;
               return;
       }
(quoted diff) +5 -0
+       /* is sending the skb back to the incoming interface allowed? */
+       if (!(gwj->flags & CGW_FLAGS_CAN_IIF_TX_OK) &&
+           skb->skb_iif == gwj->dst.dev->ifindex)
+               return;
+
       /*
        * clone the given skb, which has not been done in can_rcv()
        *
Quoted 5 lines
This works fine, when the patch from Dave is reverted.
I did not find any good solution to preserve the originating netdev over several netif_receive_skb() calls - but this skb_iif which has been made unusable ...
Well, look at struct receiver in net/can/af_can.h:
struct receiver {
        struct hlist_node list;
        struct rcu_head rcu;
        canid_t can_id;
        canid_t mask;
        unsigned long matches;
        void (*func)(struct sk_buff *, void *);
        void *data;
        char *ident;
};

Quoted 9 lines

your can_can_gw_rcv() is callback registered as ->func here.
This ->func is called from chain can_rcv->can_receive->can_rcv_filter->deliver
So just extend
->func to something like:
        void (*func)(struct sk_buff *, struct neti_device *orig_dev, void *);
and pass the orig_dev all the way through the chain.

Passing the information up to the can-gw once is not the problem.

But when this skb has to be routed to another CAN netdev it is cloned and goes down from can_can_gw_rcv() to
-> can_send(cloned_skb) -> dev_queue_xmit(cloned_skb)

And when it has been sent successfully on the CAN bus the cloned_skb is echoed back into the system via netif_rx_ni(cloned_skb).
(see http://lxr.linux.no/#linux+v3.7.2/Documentation/networking/can.txt#L177 )

This entire path - using dev_queue_xmit() / netif_rx_ni() - is reduced to a skb structure and can not deal with any orig_dev pointer.

Therefore storing the first incoming interface in skb_iif is relevant for this use-case.

Regards,
Oliver

David Miller

maintainer

2013-01-12 21:23 UTC #5 lore raw

In reply to Oliver Hartkopp

From: Oliver Hartkopp <[email protected]>
Date: Sat, 12 Jan 2013 14:48:14 +0100

Quoted 2 lines

To me it is not clear why skb_iff is needed anyway as the value should always be available via skb->dev->ifindex, right?

But all the code uses skb_iif, in particular the ipv4 routing lookups use that as the key.

It absolutely must follow whatever is skb->dev, it is a hard invariant.

I am not reverting this change.

David Miller

maintainer

2013-01-12 21:36 UTC #6 lore raw

From: David Miller <[email protected]>
Date: Sat, 12 Jan 2013 13:23:16 -0800 (PST)

Quoted 6 lines

From: Oliver Hartkopp <[email protected]>
Date: Sat, 12 Jan 2013 14:48:14 +0100
Quoted 2 lines
To me it is not clear why skb_iff is needed anyway as the value should always be available via skb->dev->ifindex, right?

Quoted 7 lines

But all the code uses skb_iif, in particular the ipv4 routing lookups use that as the key.
It absolutely must follow whatever is skb->dev, it is a hard invariant.
I am not reverting this change.

More information, because I can't believe how idiotic and ignorant people are being able this issue.

skb->dev->ifindex IS NOT the same as skb->skb_iif

Why don't you put a test into tcp_recvmsg() for packets being removed from the socket's receive queue and see if skb->dev->ifindex is the same as skb->skb_iif

Surprise, skb->dev is going to be NULL at that point.

Why?

Because on packet receive we don't take references on devices we hook into skb->dev, therefore we cannot let that pointer escape the software interrupt packet input paths.

Therefore, as a bug trap, TCP input will set skb->dev to NULL.

The only valid way to figure out the final demuxed device the packet arrived on, is therefore, via skb->skb_iif.

As per your problem with CAN, that's also rediculous. You have an SKB control block in skb->cb[] that you can put whatever values with whatever semantics you want.

Use it.

I'm not discussing this any further.

Eric Dumazet

2013-01-13 03:06 UTC #7 lore raw

PATCH 1/1 pkt_sched: namespace aware ifb

From: Eric Dumazet <[email protected]>

act_mirred needs to find the current net_ns, and struct net pointer is not provided in the call chain. We run in process context and current->nsproxy->net_ns is the needed pointer.

For ifb, things are easier, as the current ifb device can provide the net pointer immediately.

Signed-off-by Eric Dumazet <[email protected]>

---
 drivers/net/ifb.c      |    2 +-
 net/sched/act_mirred.c |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/net/ifb.c +1 -1

index 344dceb..8216438 100644
@@ -90,7 +90,7 @@ static void ri_tasklet(unsigned long dev)
 		u64_stats_update_end(&dp->tsync);
 
 		rcu_read_lock();
-		skb->dev = dev_get_by_index_rcu(&init_net, skb->skb_iif);
+		skb->dev = dev_get_by_index_rcu(dev_net(_dev), skb->skb_iif);
 		if (!skb->dev) {
 			rcu_read_unlock();
 			dev_kfree_skb(skb);

net/sched/act_mirred.c +1 -1

index 9c0fd0c..f5a7e18 100644
@@ -88,7 +88,7 @@ static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	}
 	if (parm->ifindex) {
-		dev = __dev_get_by_index(&init_net, parm->ifindex);
+		dev = __dev_get_by_index(current->nsproxy->net_ns, parm->ifindex);
 		if (dev == NULL)
 			return -ENODEV;
 		switch (dev->type) {

Benjamin LaHaise

2013-01-13 03:50 UTC #8 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Quoted 5 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>
act_mirred needs to find the current net_ns, and struct net pointer is not provided in the call chain. We run in process context and current->nsproxy->net_ns is the needed pointer.

...

I don't think this is correct. Going by the call chain, tcf_action_add can be called because of a netlink message, and that netlink message may not be in the same "struct net" as the current process. It looks like the ->init operation is going to need to have the namespace passed in for this to work correctly.

-ben

Quoted 19 lines

net/sched/act_mirred.c +1 -2
index 9c0fd0c..f5a7e18 100644
@@ -88,7 +88,7 @@ static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	}
 	if (parm->ifindex) {
-		dev = __dev_get_by_index(&init_net, parm->ifindex);
+		dev = __dev_get_by_index(current->nsproxy->net_ns, parm->ifindex);
 		if (dev == NULL)
 			return -ENODEV;
 		switch (dev->type) {
 
 
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

"Thought is the essence of where you are now."

Eric Dumazet

2013-01-13 05:50 UTC #9 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Quoted 6 lines · Benjamin LaHaise

Quoted 5 lines · Eric Dumazet
From: Eric Dumazet <[email protected]>
act_mirred needs to find the current net_ns, and struct net pointer is not provided in the call chain. We run in process context and current->nsproxy->net_ns is the needed pointer.

Quoted 7 lines

...
I don't think this is correct. Going by the call chain, tcf_action_add can be called because of a netlink message, and that netlink message may not be in the same "struct net" as the current process. It looks like the ->init operation is going to need to have the namespace passed in for this to work correctly.

But it is working in my tests.

I added a WARN and the call stack is :

[  701.522282]  [<ffffffff8108634f>] warn_slowpath_common+0x7f/0xc0
[  701.522284]  [<ffffffff810863aa>] warn_slowpath_null+0x1a/0x20
[  701.522286]  [<ffffffffa00e00e3>] tcf_mirred_init+0x43/0x340 [act_mirred]
[  701.522289]  [<ffffffff81506385>] ? __rtnl_unlock+0x15/0x20
[  701.522293]  [<ffffffff815195e8>] tcf_action_init_1+0x198/0x1e0
[  701.522295]  [<ffffffff815196c8>] tcf_action_init+0x98/0x100
[  701.522298]  [<ffffffff81517f30>] tcf_exts_validate+0x90/0xb0
[  701.522300]  [<ffffffff8151e55b>] u32_set_parms.isra.11+0x3b/0x270
[  701.522303]  [<ffffffff812ffdf0>] ? nla_parse+0x90/0xe0
[  701.522304]  [<ffffffff8151ed16>] u32_change+0x2e6/0x4c0
[  701.522306]  [<ffffffff81518432>] tc_ctl_tfilter+0x4e2/0x720
[  701.522308]  [<ffffffff815064ad>] rtnetlink_rcv_msg+0x11d/0x310
[  701.522310]  [<ffffffff81506390>] ? __rtnl_unlock+0x20/0x20
[  701.522312]  [<ffffffff81522d39>] netlink_rcv_skb+0xa9/0xd0
[  701.522314]  [<ffffffff81503875>] rtnetlink_rcv+0x25/0x40
[  701.522316]  [<ffffffff81522681>] netlink_unicast+0x1b1/0x230
[  701.522317]  [<ffffffff815229fe>] netlink_sendmsg+0x2fe/0x3b0
[  701.522321]  [<ffffffff814dbdf2>] sock_sendmsg+0xd2/0xf0
[  701.522323]  [<ffffffff814dbca0>] ? sock_recvmsg+0xe0/0x100
[  701.522326]  [<ffffffff814dd0f0>] __sys_sendmsg+0x380/0x390
[  701.522329]  [<ffffffff815b20b4>] ? __do_page_fault+0x214/0x460
[  701.522331]  [<ffffffff814df4e9>] sys_sendmsg+0x49/0x90
[  701.522334]  [<ffffffff815b68c2>] system_call_fastpath+0x16/0x1b

Could you elaborate on what could be the problem ?

We hold the RTNL, so I dont think another process could possibly call tcf_mirred_init()

Jamal Hadi Salim

maintainer

2013-01-13 14:45 UTC #10 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Quoted 5 lines · Eric Dumazet

Could you elaborate on what could be the problem ?
We hold the RTNL, so I dont think another process could possibly call tcf_mirred_init()

Eric, the point probably Ben was trying to make is not about synchronizing rather about which namespace has the right to that action config. Your change is correct for the common use of actions but does not fix the larger picture.

At the moment a dev is owned by a specific namespace; that owns a tc filter that in turn owns an action. So no problem with the change you make if all configuration follows those rules i.e something along the lines of:
===
tc filter add dev eth0 parent 1: protocol ip prio 1 u32 \
match ip dst 10.0.0.229/32 flowid 1:10 \
action mirred egress redirect dev ifb0
=====

I would say most people use the above syntax.
However, there is another way to configure actions so they can be shared[1], example control syntax:
----
tc actions add \
action police rate 1kbit burst 90k drop index 3 \
action mirred egress mirror dev eth0 index 5
----

In such a case, the "tc actions" netlink path may be entered from a different namespace than the one that is using it. Then current->nsproxy->net_ns is no longer correct.

To correct this, i think what Ben points out in passing the init() the correct namespace seem like the way to go. Feel free to make that change - otherwise i will get to it and fix it.

cheers,
jamal

[1]
You can then have multiple filters use this action like so:
===
tc filter add dev eth0 parent 1: protocol ip prio 1 u32 \
match ip dst 10.0.0.229/32 flowid 1:10 \
action police index 3
#
tc filter add dev eth0 parent ffff: protocol ip prio 6 u32 \
match ip src 10.0.0.21/32 flowid 1:16 \
action police index 3 action mirred egress mirror dev eth0
=====

Benjamin LaHaise

2013-01-13 16:41 UTC #11 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Hi folks,

Quoted 4 lines · Jamal Hadi Salim

Eric, the point probably Ben was trying to make is not about synchronizing rather about which namespace has the right to that action config. Your change is correct for the common use of actions but does not fix the larger picture.

...

Quoted 7 lines

In such a case, the "tc actions" netlink path may be entered from a different namespace than the one that is using it. Then current->nsproxy->net_ns is no longer correct.
To correct this, i think what Ben points out in passing the init() the correct namespace seem like the way to go. Feel free to make that change - otherwise i will get to it and fix it.

Yep, Jamal's right on the point I'm trying to make. To deal with this, I think we need a patch along the lines of the following to pass the 'struct net *' down to where it's needed... Please note that I've only compile tested this with all the net/sched modules enabled and a allmodconfig build. It's a bit bigger, but passing the argument down through the call chain looks simpler than trying to stuff a struct net pointer into the various structures and keep that in sync with the network device's network namespace.

-ben

Signature · 38 lines

"Thought is the essence of where you are now."

-- 
pkt_sched: namespace aware ifb v2

Eric Dumazet pointed out that act_mirred needs to find the current net_ns,
and struct net pointer is not provided in the call chain.  His original
patch made use of current->nsproxy->net_ns to find the network namespace,
but this fails to work correctly for userspace code that makes use of
netlink sockets in different network namespaces.  Instead, pass the
"struct net *" down along the call chain to where it is needed.

Signed-off-by: Benjamin LaHaise <[email protected]>
---
 drivers/net/ifb.c         |    2 +-
 include/net/act_api.h     |   12 +++++++++---
 include/net/pkt_cls.h     |    7 ++++---
 include/net/sch_generic.h |    2 +-
 net/sched/act_api.c       |   18 ++++++++++--------
 net/sched/act_csum.c      |    2 +-
 net/sched/act_gact.c      |    5 +++--
 net/sched/act_ipt.c       |    2 +-
 net/sched/act_mirred.c    |    7 ++++---
 net/sched/act_nat.c       |    2 +-
 net/sched/act_pedit.c     |    5 +++--
 net/sched/act_police.c    |    5 +++--
 net/sched/act_simple.c    |    5 +++--
 net/sched/act_skbedit.c   |    5 +++--
 net/sched/cls_api.c       |   11 ++++++-----
 net/sched/cls_basic.c     |   13 +++++++------
 net/sched/cls_cgroup.c    |    5 +++--
 net/sched/cls_flow.c      |    4 ++--
 net/sched/cls_fw.c        |   10 +++++-----
 net/sched/cls_route.c     |   15 ++++++++-------
 net/sched/cls_rsvp.h      |    4 ++--
 net/sched/cls_tcindex.c   |   14 ++++++++------
 net/sched/cls_u32.c       |   13 +++++++------
 23 files changed, 95 insertions(+), 73 deletions(-)

drivers/net/ifb.c +1 -1

index 344dceb..8216438 100644
@@ -90,7 +90,7 @@ static void ri_tasklet(unsigned long dev)
 		u64_stats_update_end(&dp->tsync);
 
 		rcu_read_lock();
-		skb->dev = dev_get_by_index_rcu(&init_net, skb->skb_iif);
+		skb->dev = dev_get_by_index_rcu(dev_net(_dev), skb->skb_iif);
 		if (!skb->dev) {
 			rcu_read_unlock();
 			dev_kfree_skb(skb);

include/net/act_api.h +9 -3

index c739531..112c25c 100644
@@ -91,7 +91,9 @@ struct tc_action_ops {
 	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
 	int     (*cleanup)(struct tc_action *, int bind);
 	int     (*lookup)(struct tc_action *, u32);
-	int     (*init)(struct nlattr *, struct nlattr *, struct tc_action *, int , int);
+	int     (*init)(struct net *net, struct nlattr *nla,
+			struct nlattr *est, struct tc_action *act, int ovr,
+			int bind);
 	int     (*walk)(struct sk_buff *, struct netlink_callback *, int, struct tc_action *);
 };

@@ -116,8 +118,12 @@ extern int tcf_register_action(struct tc_action_ops *a);
 extern int tcf_unregister_action(struct tc_action_ops *a);
 extern void tcf_action_destroy(struct tc_action *a, int bind);
 extern int tcf_action_exec(struct sk_buff *skb, const struct tc_action *a, struct tcf_result *res);
-extern struct tc_action *tcf_action_init(struct nlattr *nla, struct nlattr *est, char *n, int ovr, int bind);
-extern struct tc_action *tcf_action_init_1(struct nlattr *nla, struct nlattr *est, char *n, int ovr, int bind);
+extern struct tc_action *tcf_action_init(struct net *net, struct nlattr *nla,
+					 struct nlattr *est, char *n, int ovr,
+					 int bind);
+extern struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
+					   struct nlattr *est, char *n, int ovr,
+					   int bind);
 extern int tcf_action_dump(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);

include/net/pkt_cls.h +4 -3

index 9fcc680..1317450 100644
@@ -126,9 +126,10 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 	return 0;
 }
 
-extern int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
-	                     struct nlattr *rate_tlv, struct tcf_exts *exts,
-	                     const struct tcf_ext_map *map);
+extern int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
+			     struct nlattr **tb, struct nlattr *rate_tlv,
+			     struct tcf_exts *exts,
+			     const struct tcf_ext_map *map);
 extern void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
 extern void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
 	                     struct tcf_exts *src);

include/net/sch_generic.h +1 -1

index 1540f9c..2d06c2a 100644
@@ -195,7 +195,7 @@ struct tcf_proto_ops {
 
 	unsigned long		(*get)(struct tcf_proto*, u32 handle);
 	void			(*put)(struct tcf_proto*, unsigned long);
-	int			(*change)(struct sk_buff *,
+	int			(*change)(struct net *net, struct sk_buff *,
 					struct tcf_proto*, unsigned long,
 					u32 handle, struct nlattr **,
 					unsigned long *);

net/sched/act_api.c +10 -8

index 65d240c..8579c4b 100644
@@ -485,8 +485,9 @@ errout:
 	return err;
 }
 
-struct tc_action *tcf_action_init_1(struct nlattr *nla, struct nlattr *est,
-				    char *name, int ovr, int bind)
+struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
+				    struct nlattr *est, char *name, int ovr,
+				    int bind)
 {
 	struct tc_action *a;
 	struct tc_action_ops *a_o;

@@ -542,9 +543,9 @@ struct tc_action *tcf_action_init_1(struct nlattr *nla, struct nlattr *est,
 
 	/* backward compatibility for policer */
 	if (name == NULL)
-		err = a_o->init(tb[TCA_ACT_OPTIONS], est, a, ovr, bind);
+		err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, a, ovr, bind);
 	else
-		err = a_o->init(nla, est, a, ovr, bind);
+		err = a_o->init(net, nla, est, a, ovr, bind);
 	if (err < 0)
 		goto err_free;

@@ -566,8 +567,9 @@ err_out:
 	return ERR_PTR(err);
 }
 
-struct tc_action *tcf_action_init(struct nlattr *nla, struct nlattr *est,
-				  char *name, int ovr, int bind)
+struct tc_action *tcf_action_init(struct net *net, struct nlattr *nla,
+				  struct nlattr *est, char *name, int ovr,
+				  int bind)
 {
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *head = NULL, *act, *act_prev = NULL;

@@ -579,7 +581,7 @@ struct tc_action *tcf_action_init(struct nlattr *nla, struct nlattr *est,
 		return ERR_PTR(err);
 
 	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
-		act = tcf_action_init_1(tb[i], est, name, ovr, bind);
+		act = tcf_action_init_1(net, tb[i], est, name, ovr, bind);
 		if (IS_ERR(act))
 			goto err;
 		act->order = i;

@@ -960,7 +962,7 @@ tcf_action_add(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	struct tc_action *a;
 	u32 seq = n->nlmsg_seq;
 
-	act = tcf_action_init(nla, NULL, NULL, ovr, 0);
+	act = tcf_action_init(net, nla, NULL, NULL, ovr, 0);
 	if (act == NULL)
 		goto done;
 	if (IS_ERR(act)) {

net/sched/act_csum.c +1 -1

index 2c8ad7c..08fa1e8 100644
@@ -51,7 +51,7 @@ static const struct nla_policy csum_policy[TCA_CSUM_MAX + 1] = {
 	[TCA_CSUM_PARMS] = { .len = sizeof(struct tc_csum), },
 };
 
-static int tcf_csum_init(struct nlattr *nla, struct nlattr *est,
+static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 			 struct tc_action *a, int ovr, int bind)
 {
 	struct nlattr *tb[TCA_CSUM_MAX + 1];

net/sched/act_gact.c +3 -2

index 05d60859..fd2b3cf 100644
@@ -58,8 +58,9 @@ static const struct nla_policy gact_policy[TCA_GACT_MAX + 1] = {
 	[TCA_GACT_PROB]		= { .len = sizeof(struct tc_gact_p) },
 };
 
-static int tcf_gact_init(struct nlattr *nla, struct nlattr *est,
-			 struct tc_action *a, int ovr, int bind)
+static int tcf_gact_init(struct net *net, struct nlattr *nla,
+			 struct nlattr *est, struct tc_action *a,
+			 int ovr, int bind)
 {
 	struct nlattr *tb[TCA_GACT_MAX + 1];
 	struct tc_gact *parm;

net/sched/act_ipt.c +1 -1

index 58fb3c7..0fb9e3f 100644
@@ -102,7 +102,7 @@ static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
 	[TCA_IPT_TARG]	= { .len = sizeof(struct xt_entry_target) },
 };
 
-static int tcf_ipt_init(struct nlattr *nla, struct nlattr *est,
+static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 			struct tc_action *a, int ovr, int bind)
 {
 	struct nlattr *tb[TCA_IPT_MAX + 1];

net/sched/act_mirred.c +4 -3

index 9c0fd0c..5d676ed 100644
@@ -62,8 +62,9 @@ static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
 	[TCA_MIRRED_PARMS]	= { .len = sizeof(struct tc_mirred) },
 };
 
-static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
-			   struct tc_action *a, int ovr, int bind)
+static int tcf_mirred_init(struct net *net, struct nlattr *nla,
+			   struct nlattr *est, struct tc_action *a, int ovr,
+			   int bind)
 {
 	struct nlattr *tb[TCA_MIRRED_MAX + 1];
 	struct tc_mirred *parm;

@@ -88,7 +89,7 @@ static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	}
 	if (parm->ifindex) {
-		dev = __dev_get_by_index(&init_net, parm->ifindex);
+		dev = __dev_get_by_index(net, parm->ifindex);
 		if (dev == NULL)
 			return -ENODEV;
 		switch (dev->type) {

net/sched/act_nat.c +1 -1

index b5d029e..876f0ef 100644
@@ -44,7 +44,7 @@ static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
 	[TCA_NAT_PARMS]	= { .len = sizeof(struct tc_nat) },
 };
 
-static int tcf_nat_init(struct nlattr *nla, struct nlattr *est,
+static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 			struct tc_action *a, int ovr, int bind)
 {
 	struct nlattr *tb[TCA_NAT_MAX + 1];

net/sched/act_pedit.c +3 -2

index 45c53ab..0c3fadd 100644
@@ -38,8 +38,9 @@ static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = {
 	[TCA_PEDIT_PARMS]	= { .len = sizeof(struct tc_pedit) },
 };
 
-static int tcf_pedit_init(struct nlattr *nla, struct nlattr *est,
-			  struct tc_action *a, int ovr, int bind)
+static int tcf_pedit_init(struct net *net, struct nlattr *nla,
+			  struct nlattr *est, struct tc_action *a,
+			  int ovr, int bind)
 {
 	struct nlattr *tb[TCA_PEDIT_MAX + 1];
 	struct tc_pedit *parm;

net/sched/act_police.c +3 -2

index a9de232..8dbd695 100644
@@ -130,8 +130,9 @@ static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
 	[TCA_POLICE_RESULT]	= { .type = NLA_U32 },
 };
 
-static int tcf_act_police_locate(struct nlattr *nla, struct nlattr *est,
-				 struct tc_action *a, int ovr, int bind)
+static int tcf_act_police_locate(struct net *net, struct nlattr *nla,
+				 struct nlattr *est, struct tc_action *a,
+				 int ovr, int bind)
 {
 	unsigned int h;
 	int ret = 0, err;

net/sched/act_simple.c +3 -2

index 3714f60..7725eb4 100644
@@ -95,8 +95,9 @@ static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
 	[TCA_DEF_DATA]	= { .type = NLA_STRING, .len = SIMP_MAX_DATA },
 };
 
-static int tcf_simp_init(struct nlattr *nla, struct nlattr *est,
-			 struct tc_action *a, int ovr, int bind)
+static int tcf_simp_init(struct net *net, struct nlattr *nla,
+			 struct nlattr *est, struct tc_action *a,
+			 int ovr, int bind)
 {
 	struct nlattr *tb[TCA_DEF_MAX + 1];
 	struct tc_defact *parm;

net/sched/act_skbedit.c +3 -2

index 476e0fa..cb42211 100644
@@ -67,8 +67,9 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
 	[TCA_SKBEDIT_MARK]		= { .len = sizeof(u32) },
 };
 
-static int tcf_skbedit_init(struct nlattr *nla, struct nlattr *est,
-			 struct tc_action *a, int ovr, int bind)
+static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
+			    struct nlattr *est, struct tc_action *a,
+			    int ovr, int bind)
 {
 	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
 	struct tc_skbedit *parm;

net/sched/cls_api.c +6 -5

index ff55ed6..964f5e4 100644
@@ -321,7 +321,7 @@ replay:
 		}
 	}
 
-	err = tp->ops->change(skb, tp, cl, t->tcm_handle, tca, &fh);
+	err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh);
 	if (err == 0) {
 		if (tp_created) {
 			spin_lock_bh(root_lock);

@@ -508,7 +508,7 @@ void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
 }
 EXPORT_SYMBOL(tcf_exts_destroy);
 
-int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
+int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 		  struct nlattr *rate_tlv, struct tcf_exts *exts,
 		  const struct tcf_ext_map *map)
 {

@@ -519,7 +519,7 @@ int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
 		struct tc_action *act;
 
 		if (map->police && tb[map->police]) {
-			act = tcf_action_init_1(tb[map->police], rate_tlv,
+			act = tcf_action_init_1(net, tb[map->police], rate_tlv,
 						"police", TCA_ACT_NOREPLACE,
 						TCA_ACT_BIND);
 			if (IS_ERR(act))

@@ -528,8 +528,9 @@ int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
 			act->type = TCA_OLD_COMPAT;
 			exts->action = act;
 		} else if (map->action && tb[map->action]) {
-			act = tcf_action_init(tb[map->action], rate_tlv, NULL,
-					      TCA_ACT_NOREPLACE, TCA_ACT_BIND);
+			act = tcf_action_init(net, tb[map->action], rate_tlv,
+					      NULL, TCA_ACT_NOREPLACE,
+					      TCA_ACT_BIND);
 			if (IS_ERR(act))
 				return PTR_ERR(act);

net/sched/cls_basic.c +7 -6

index 344a11b..d76a35d 100644
@@ -132,15 +132,16 @@ static const struct nla_policy basic_policy[TCA_BASIC_MAX + 1] = {
 	[TCA_BASIC_EMATCHES]	= { .type = NLA_NESTED },
 };
 
-static int basic_set_parms(struct tcf_proto *tp, struct basic_filter *f,
-			   unsigned long base, struct nlattr **tb,
+static int basic_set_parms(struct net *net, struct tcf_proto *tp,
+			   struct basic_filter *f, unsigned long base,
+			   struct nlattr **tb,
 			   struct nlattr *est)
 {
 	int err = -EINVAL;
 	struct tcf_exts e;
 	struct tcf_ematch_tree t;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &basic_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &basic_ext_map);
 	if (err < 0)
 		return err;

@@ -162,7 +163,7 @@ errout:
 	return err;
 }
 
-static int basic_change(struct sk_buff *in_skb,
+static int basic_change(struct net *net, struct sk_buff *in_skb,
 			struct tcf_proto *tp, unsigned long base, u32 handle,
 			struct nlattr **tca, unsigned long *arg)
 {

@@ -182,7 +183,7 @@ static int basic_change(struct sk_buff *in_skb,
 	if (f != NULL) {
 		if (handle && f->handle != handle)
 			return -EINVAL;
-		return basic_set_parms(tp, f, base, tb, tca[TCA_RATE]);
+		return basic_set_parms(net, tp, f, base, tb, tca[TCA_RATE]);
 	}
 
 	err = -ENOBUFS;

@@ -208,7 +209,7 @@ static int basic_change(struct sk_buff *in_skb,
 		f->handle = head->hgenerator;
 	}
 
-	err = basic_set_parms(tp, f, base, tb, tca[TCA_RATE]);
+	err = basic_set_parms(net, tp, f, base, tb, tca[TCA_RATE]);
 	if (err < 0)
 		goto errout;

net/sched/cls_cgroup.c +3 -2

index 6db7855..3a294eb 100644
@@ -178,7 +178,7 @@ static const struct nla_policy cgroup_policy[TCA_CGROUP_MAX + 1] = {
 	[TCA_CGROUP_EMATCHES]	= { .type = NLA_NESTED },
 };
 
-static int cls_cgroup_change(struct sk_buff *in_skb,
+static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 			     struct tcf_proto *tp, unsigned long base,
 			     u32 handle, struct nlattr **tca,
 			     unsigned long *arg)

@@ -215,7 +215,8 @@ static int cls_cgroup_change(struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &cgroup_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e,
+				&cgroup_ext_map);
 	if (err < 0)
 		return err;

net/sched/cls_flow.c +2 -2

index ce82d0c..e6c1427 100644
@@ -351,7 +351,7 @@ static const struct nla_policy flow_policy[TCA_FLOW_MAX + 1] = {
 	[TCA_FLOW_PERTURB]	= { .type = NLA_U32 },
 };
 
-static int flow_change(struct sk_buff *in_skb, 
+static int flow_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle, struct nlattr **tca,
 		       unsigned long *arg)

@@ -397,7 +397,7 @@ static int flow_change(struct sk_buff *in_skb,
 			return -EOPNOTSUPP;
 	}
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &flow_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &flow_ext_map);
 	if (err < 0)
 		return err;

net/sched/cls_fw.c +5 -5

index 4075a0a..1135d82 100644
@@ -192,7 +192,7 @@ static const struct nla_policy fw_policy[TCA_FW_MAX + 1] = {
 };
 
 static int
-fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
+fw_change_attrs(struct net *net, struct tcf_proto *tp, struct fw_filter *f,
 	struct nlattr **tb, struct nlattr **tca, unsigned long base)
 {
 	struct fw_head *head = (struct fw_head *)tp->root;

@@ -200,7 +200,7 @@ fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
 	u32 mask;
 	int err;
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &fw_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &fw_ext_map);
 	if (err < 0)
 		return err;

@@ -233,7 +233,7 @@ errout:
 	return err;
 }
 
-static int fw_change(struct sk_buff *in_skb,
+static int fw_change(struct net *net, struct sk_buff *in_skb,
 		     struct tcf_proto *tp, unsigned long base,
 		     u32 handle,
 		     struct nlattr **tca,

@@ -255,7 +255,7 @@ static int fw_change(struct sk_buff *in_skb,
 	if (f != NULL) {
 		if (f->id != handle && handle)
 			return -EINVAL;
-		return fw_change_attrs(tp, f, tb, tca, base);
+		return fw_change_attrs(net, tp, f, tb, tca, base);
 	}
 
 	if (!handle)

@@ -282,7 +282,7 @@ static int fw_change(struct sk_buff *in_skb,
 
 	f->id = handle;
 
-	err = fw_change_attrs(tp, f, tb, tca, base);
+	err = fw_change_attrs(net, tp, f, tb, tca, base);
 	if (err < 0)
 		goto errout;

net/sched/cls_route.c +8 -7

index c10d57b..37da567 100644
@@ -335,9 +335,10 @@ static const struct nla_policy route4_policy[TCA_ROUTE4_MAX + 1] = {
 	[TCA_ROUTE4_IIF]	= { .type = NLA_U32 },
 };
 
-static int route4_set_parms(struct tcf_proto *tp, unsigned long base,
-	struct route4_filter *f, u32 handle, struct route4_head *head,
-	struct nlattr **tb, struct nlattr *est, int new)
+static int route4_set_parms(struct net *net, struct tcf_proto *tp,
+			    unsigned long base, struct route4_filter *f,
+			    u32 handle, struct route4_head *head,
+			    struct nlattr **tb, struct nlattr *est, int new)
 {
 	int err;
 	u32 id = 0, to = 0, nhandle = 0x8000;

@@ -346,7 +347,7 @@ static int route4_set_parms(struct tcf_proto *tp, unsigned long base,
 	struct route4_bucket *b;
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &route_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &route_ext_map);
 	if (err < 0)
 		return err;

@@ -427,7 +428,7 @@ errout:
 	return err;
 }
 
-static int route4_change(struct sk_buff *in_skb,
+static int route4_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle,
 		       struct nlattr **tca,

@@ -457,7 +458,7 @@ static int route4_change(struct sk_buff *in_skb,
 		if (f->bkt)
 			old_handle = f->handle;
 
-		err = route4_set_parms(tp, base, f, handle, head, tb,
+		err = route4_set_parms(net, tp, base, f, handle, head, tb,
 			tca[TCA_RATE], 0);
 		if (err < 0)
 			return err;

@@ -480,7 +481,7 @@ static int route4_change(struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout;
 
-	err = route4_set_parms(tp, base, f, handle, head, tb,
+	err = route4_set_parms(net, tp, base, f, handle, head, tb,
 		tca[TCA_RATE], 1);
 	if (err < 0)
 		goto errout;

net/sched/cls_rsvp.h +2 -2

index 494bbb9..252d8b0 100644
@@ -416,7 +416,7 @@ static const struct nla_policy rsvp_policy[TCA_RSVP_MAX + 1] = {
 	[TCA_RSVP_PINFO]	= { .len = sizeof(struct tc_rsvp_pinfo) },
 };
 
-static int rsvp_change(struct sk_buff *in_skb,
+static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle,
 		       struct nlattr **tca,

@@ -440,7 +440,7 @@ static int rsvp_change(struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &rsvp_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &rsvp_ext_map);
 	if (err < 0)
 		return err;

net/sched/cls_tcindex.c +8 -6

index a1293b4..b86535a 100644
@@ -197,9 +197,10 @@ static const struct nla_policy tcindex_policy[TCA_TCINDEX_MAX + 1] = {
 };
 
 static int
-tcindex_set_parms(struct tcf_proto *tp, unsigned long base, u32 handle,
-		  struct tcindex_data *p, struct tcindex_filter_result *r,
-		  struct nlattr **tb, struct nlattr *est)
+tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
+		  u32 handle, struct tcindex_data *p,
+		  struct tcindex_filter_result *r, struct nlattr **tb,
+		 struct nlattr *est)
 {
 	int err, balloc = 0;
 	struct tcindex_filter_result new_filter_result, *old_r = r;

@@ -208,7 +209,7 @@ tcindex_set_parms(struct tcf_proto *tp, unsigned long base, u32 handle,
 	struct tcindex_filter *f = NULL; /* make gcc behave */
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &tcindex_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &tcindex_ext_map);
 	if (err < 0)
 		return err;

@@ -332,7 +333,7 @@ errout:
 }
 
 static int
-tcindex_change(struct sk_buff *in_skb,
+tcindex_change(struct net *net, struct sk_buff *in_skb,
 	       struct tcf_proto *tp, unsigned long base, u32 handle,
 	       struct nlattr **tca, unsigned long *arg)
 {

@@ -353,7 +354,8 @@ tcindex_change(struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	return tcindex_set_parms(tp, base, handle, p, r, tb, tca[TCA_RATE]);
+	return tcindex_set_parms(net, tp, base, handle, p, r, tb,
+				 tca[TCA_RATE]);
 }

net/sched/cls_u32.c +7 -6

index c7c27bc..eb07a1e 100644
@@ -488,15 +488,15 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 	[TCA_U32_MARK]		= { .len = sizeof(struct tc_u32_mark) },
 };
 
-static int u32_set_parms(struct tcf_proto *tp, unsigned long base,
-			 struct tc_u_hnode *ht,
+static int u32_set_parms(struct net *net, struct tcf_proto *tp,
+			 unsigned long base, struct tc_u_hnode *ht,
 			 struct tc_u_knode *n, struct nlattr **tb,
 			 struct nlattr *est)
 {
 	int err;
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &u32_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &u32_ext_map);
 	if (err < 0)
 		return err;

@@ -544,7 +544,7 @@ errout:
 	return err;
 }
 
-static int u32_change(struct sk_buff *in_skb,
+static int u32_change(struct net *net, struct sk_buff *in_skb,
 		      struct tcf_proto *tp, unsigned long base, u32 handle,
 		      struct nlattr **tca,
 		      unsigned long *arg)

@@ -570,7 +570,8 @@ static int u32_change(struct sk_buff *in_skb,
 		if (TC_U32_KEY(n->handle) == 0)
 			return -EINVAL;
 
-		return u32_set_parms(tp, base, n->ht_up, n, tb, tca[TCA_RATE]);
+		return u32_set_parms(net, tp, base, n->ht_up, n, tb,
+				     tca[TCA_RATE]);
 	}
 
 	if (tb[TCA_U32_DIVISOR]) {

@@ -656,7 +657,7 @@ static int u32_change(struct sk_buff *in_skb,
 	}
 #endif
 
-	err = u32_set_parms(tp, base, ht, n, tb, tca[TCA_RATE]);
+	err = u32_set_parms(net, tp, base, ht, n, tb, tca[TCA_RATE]);
 	if (err == 0) {
 		struct tc_u_knode **ins;
 		for (ins = &ht->ht[TC_U32_HASH(handle)]; *ins; ins = &(*ins)->next)

Eric Dumazet

2013-01-13 16:57 UTC #12 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Quoted 25 lines · Benjamin LaHaise

Hi folks,
Quoted 4 lines · Jamal Hadi Salim
Eric, the point probably Ben was trying to make is not about synchronizing rather about which namespace has the right to that action config. Your change is correct for the common use of actions but does not fix the larger picture.
...
Quoted 7 lines
In such a case, the "tc actions" netlink path may be entered from a different namespace than the one that is using it. Then current->nsproxy->net_ns is no longer correct.
To correct this, i think what Ben points out in passing the init() the correct namespace seem like the way to go. Feel free to make that change - otherwise i will get to it and fix it.
Yep, Jamal's right on the point I'm trying to make. To deal with this, I think we need a patch along the lines of the following to pass the 'struct net *' down to where it's needed... Please note that I've only compile tested this with all the net/sched modules enabled and a allmodconfig build. It's a bit bigger, but passing the argument down through the call chain looks simpler than trying to stuff a struct net pointer into the various structures and keep that in sync with the network device's network namespace.

Quoted 16 lines

-ben

Signature · 14 lines

"Thought is the essence of where you are now."

-- 
pkt_sched: namespace aware ifb v2

Eric Dumazet pointed out that act_mirred needs to find the current net_ns,
and struct net pointer is not provided in the call chain.  His original
patch made use of current->nsproxy->net_ns to find the network namespace,
but this fails to work correctly for userspace code that makes use of
netlink sockets in different network namespaces.  Instead, pass the
"struct net *" down along the call chain to where it is needed.

Signed-off-by: Benjamin LaHaise <[email protected]>
---

OK I'll test it at the end of the (sunny) day.

Thanks

Jamal Hadi Salim

maintainer

2013-01-13 17:26 UTC #13 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Quoted 4 lines · Eric Dumazet

Quoted 2 lines
Signed-off-by Benjamin LaHaise <[email protected]>
---
OK I'll test it at the end of the (sunny) day.

And from my side this looks good.
I think the ifb piece is a separate patch. Please add my ack/signoff when you make the submission.

[Sorry dont have time to test it today or next 2-3 days (but you can be assured i will test).]

cheers,
jamal

Eric Dumazet

2013-01-14 05:40 UTC #14 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

In reply to Eric Dumazet

Quoted 11 lines · Eric Dumazet

Quoted 9 lines · Benjamin LaHaise
pkt_sched: namespace aware ifb v2
Eric Dumazet pointed out that act_mirred needs to find the current net_ns, and struct net pointer is not provided in the call chain. His original patch made use of current->nsproxy->net_ns to find the network namespace, but this fails to work correctly for userspace code that makes use of netlink sockets in different network namespaces. Instead, pass the "struct net *" down along the call chain to where it is needed.

Quoted 5 lines

Quoted 2 lines
Signed-off-by Benjamin LaHaise <[email protected]>
---
OK I'll test it at the end of the (sunny) day.

Could you resend the patch without the ifb part, I tested it and it worked well for me, thanks !

Eric Dumazet

2013-01-13 17:46 UTC #15 lore raw

PATCH 1/1 ifb: dont hard code inet_net use

In reply to Jamal Hadi Salim

From: Eric Dumazet <[email protected]>

ifb should lookup devices in the appropriate namespace.

Signed-off-by Eric Dumazet <[email protected]>

Cc Jamal Hadi Salim <[email protected]>

Cc Benjamin LaHaise <[email protected]>

---
 drivers/net/ifb.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/net/ifb.c +1 -1

index 344dceb..8216438 100644
@@ -90,7 +90,7 @@ static void ri_tasklet(unsigned long dev)
 		u64_stats_update_end(&dp->tsync);
 
 		rcu_read_lock();
-		skb->dev = dev_get_by_index_rcu(&init_net, skb->skb_iif);
+		skb->dev = dev_get_by_index_rcu(dev_net(_dev), skb->skb_iif);
 		if (!skb->dev) {
 			rcu_read_unlock();
 			dev_kfree_skb(skb);

Jamal Hadi Salim

maintainer Acked-by on v1 1/1

2013-01-14 13:13 UTC #16 lore raw

Re: [PATCH net-next] ifb: dont hard code inet_net use

Quoted 5 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>
ifb should lookup devices in the appropriate namespace.
Signed-off-by Eric Dumazet <[email protected]>

Acked-by Jamal Hadi Salim <[email protected]>

cheers,
jamal

Benjamin LaHaise

2013-01-14 15:15 UTC #17 lore raw

PATCH 1/1 pkt_sched: namespace aware act_mirred

Eric Dumazet pointed out that act_mirred needs to find the current net_ns, and struct net pointer is not provided in the call chain. His original patch made use of current->nsproxy->net_ns to find the network namespace, but this fails to work correctly for userspace code that makes use of netlink sockets in different network namespaces. Instead, pass the "struct net *" down along the call chain to where it is needed.

This version removes the ifb changes as Eric has submitted that patch separately, but is otherwise identical to the previous version.

Signed-off-by Benjamin LaHaise <[email protected]>

Tested-by Eric Dumazet <[email protected]>

Acked-by Jamal Hadi Salim <[email protected]>

---
 include/net/act_api.h     |   12 +++++++++---
 include/net/pkt_cls.h     |    7 ++++---
 include/net/sch_generic.h |    2 +-
 net/sched/act_api.c       |   18 ++++++++++--------
 net/sched/act_csum.c      |    2 +-
 net/sched/act_gact.c      |    5 +++--
 net/sched/act_ipt.c       |    2 +-
 net/sched/act_mirred.c    |    7 ++++---
 net/sched/act_nat.c       |    2 +-
 net/sched/act_pedit.c     |    5 +++--
 net/sched/act_police.c    |    5 +++--
 net/sched/act_simple.c    |    5 +++--
 net/sched/act_skbedit.c   |    5 +++--
 net/sched/cls_api.c       |   11 ++++++-----
 net/sched/cls_basic.c     |   13 +++++++------
 net/sched/cls_cgroup.c    |    5 +++--
 net/sched/cls_flow.c      |    4 ++--
 net/sched/cls_fw.c        |   10 +++++-----
 net/sched/cls_route.c     |   15 ++++++++-------
 net/sched/cls_rsvp.h      |    4 ++--
 net/sched/cls_tcindex.c   |   14 ++++++++------
 net/sched/cls_u32.c       |   13 +++++++------
 22 files changed, 94 insertions(+), 72 deletions(-)

include/net/act_api.h +9 -3

index c739531..112c25c 100644
@@ -91,7 +91,9 @@ struct tc_action_ops {
 	int     (*dump)(struct sk_buff *, struct tc_action *, int, int);
 	int     (*cleanup)(struct tc_action *, int bind);
 	int     (*lookup)(struct tc_action *, u32);
-	int     (*init)(struct nlattr *, struct nlattr *, struct tc_action *, int , int);
+	int     (*init)(struct net *net, struct nlattr *nla,
+			struct nlattr *est, struct tc_action *act, int ovr,
+			int bind);
 	int     (*walk)(struct sk_buff *, struct netlink_callback *, int, struct tc_action *);
 };

@@ -116,8 +118,12 @@ extern int tcf_register_action(struct tc_action_ops *a);
 extern int tcf_unregister_action(struct tc_action_ops *a);
 extern void tcf_action_destroy(struct tc_action *a, int bind);
 extern int tcf_action_exec(struct sk_buff *skb, const struct tc_action *a, struct tcf_result *res);
-extern struct tc_action *tcf_action_init(struct nlattr *nla, struct nlattr *est, char *n, int ovr, int bind);
-extern struct tc_action *tcf_action_init_1(struct nlattr *nla, struct nlattr *est, char *n, int ovr, int bind);
+extern struct tc_action *tcf_action_init(struct net *net, struct nlattr *nla,
+					 struct nlattr *est, char *n, int ovr,
+					 int bind);
+extern struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
+					   struct nlattr *est, char *n, int ovr,
+					   int bind);
 extern int tcf_action_dump(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);

include/net/pkt_cls.h +4 -3

index 9fcc680..1317450 100644
@@ -126,9 +126,10 @@ tcf_exts_exec(struct sk_buff *skb, struct tcf_exts *exts,
 	return 0;
 }
 
-extern int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
-	                     struct nlattr *rate_tlv, struct tcf_exts *exts,
-	                     const struct tcf_ext_map *map);
+extern int tcf_exts_validate(struct net *net, struct tcf_proto *tp,
+			     struct nlattr **tb, struct nlattr *rate_tlv,
+			     struct tcf_exts *exts,
+			     const struct tcf_ext_map *map);
 extern void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts);
 extern void tcf_exts_change(struct tcf_proto *tp, struct tcf_exts *dst,
 	                     struct tcf_exts *src);

include/net/sch_generic.h +1 -1

index 1540f9c..2d06c2a 100644
@@ -195,7 +195,7 @@ struct tcf_proto_ops {
 
 	unsigned long		(*get)(struct tcf_proto*, u32 handle);
 	void			(*put)(struct tcf_proto*, unsigned long);
-	int			(*change)(struct sk_buff *,
+	int			(*change)(struct net *net, struct sk_buff *,
 					struct tcf_proto*, unsigned long,
 					u32 handle, struct nlattr **,
 					unsigned long *);

net/sched/act_api.c +10 -8

index 65d240c..8579c4b 100644
@@ -485,8 +485,9 @@ errout:
 	return err;
 }
 
-struct tc_action *tcf_action_init_1(struct nlattr *nla, struct nlattr *est,
-				    char *name, int ovr, int bind)
+struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
+				    struct nlattr *est, char *name, int ovr,
+				    int bind)
 {
 	struct tc_action *a;
 	struct tc_action_ops *a_o;

@@ -542,9 +543,9 @@ struct tc_action *tcf_action_init_1(struct nlattr *nla, struct nlattr *est,
 
 	/* backward compatibility for policer */
 	if (name == NULL)
-		err = a_o->init(tb[TCA_ACT_OPTIONS], est, a, ovr, bind);
+		err = a_o->init(net, tb[TCA_ACT_OPTIONS], est, a, ovr, bind);
 	else
-		err = a_o->init(nla, est, a, ovr, bind);
+		err = a_o->init(net, nla, est, a, ovr, bind);
 	if (err < 0)
 		goto err_free;

@@ -566,8 +567,9 @@ err_out:
 	return ERR_PTR(err);
 }
 
-struct tc_action *tcf_action_init(struct nlattr *nla, struct nlattr *est,
-				  char *name, int ovr, int bind)
+struct tc_action *tcf_action_init(struct net *net, struct nlattr *nla,
+				  struct nlattr *est, char *name, int ovr,
+				  int bind)
 {
 	struct nlattr *tb[TCA_ACT_MAX_PRIO + 1];
 	struct tc_action *head = NULL, *act, *act_prev = NULL;

@@ -579,7 +581,7 @@ struct tc_action *tcf_action_init(struct nlattr *nla, struct nlattr *est,
 		return ERR_PTR(err);
 
 	for (i = 1; i <= TCA_ACT_MAX_PRIO && tb[i]; i++) {
-		act = tcf_action_init_1(tb[i], est, name, ovr, bind);
+		act = tcf_action_init_1(net, tb[i], est, name, ovr, bind);
 		if (IS_ERR(act))
 			goto err;
 		act->order = i;

@@ -960,7 +962,7 @@ tcf_action_add(struct net *net, struct nlattr *nla, struct nlmsghdr *n,
 	struct tc_action *a;
 	u32 seq = n->nlmsg_seq;
 
-	act = tcf_action_init(nla, NULL, NULL, ovr, 0);
+	act = tcf_action_init(net, nla, NULL, NULL, ovr, 0);
 	if (act == NULL)
 		goto done;
 	if (IS_ERR(act)) {

net/sched/act_csum.c +1 -1

index 2c8ad7c..08fa1e8 100644
@@ -51,7 +51,7 @@ static const struct nla_policy csum_policy[TCA_CSUM_MAX + 1] = {
 	[TCA_CSUM_PARMS] = { .len = sizeof(struct tc_csum), },
 };
 
-static int tcf_csum_init(struct nlattr *nla, struct nlattr *est,
+static int tcf_csum_init(struct net *n, struct nlattr *nla, struct nlattr *est,
 			 struct tc_action *a, int ovr, int bind)
 {
 	struct nlattr *tb[TCA_CSUM_MAX + 1];

net/sched/act_gact.c +3 -2

index 05d60859..fd2b3cf 100644
@@ -58,8 +58,9 @@ static const struct nla_policy gact_policy[TCA_GACT_MAX + 1] = {
 	[TCA_GACT_PROB]		= { .len = sizeof(struct tc_gact_p) },
 };
 
-static int tcf_gact_init(struct nlattr *nla, struct nlattr *est,
-			 struct tc_action *a, int ovr, int bind)
+static int tcf_gact_init(struct net *net, struct nlattr *nla,
+			 struct nlattr *est, struct tc_action *a,
+			 int ovr, int bind)
 {
 	struct nlattr *tb[TCA_GACT_MAX + 1];
 	struct tc_gact *parm;

net/sched/act_ipt.c +1 -1

index 58fb3c7..0fb9e3f 100644
@@ -102,7 +102,7 @@ static const struct nla_policy ipt_policy[TCA_IPT_MAX + 1] = {
 	[TCA_IPT_TARG]	= { .len = sizeof(struct xt_entry_target) },
 };
 
-static int tcf_ipt_init(struct nlattr *nla, struct nlattr *est,
+static int tcf_ipt_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 			struct tc_action *a, int ovr, int bind)
 {
 	struct nlattr *tb[TCA_IPT_MAX + 1];

net/sched/act_mirred.c +4 -3

index 9c0fd0c..5d676ed 100644
@@ -62,8 +62,9 @@ static const struct nla_policy mirred_policy[TCA_MIRRED_MAX + 1] = {
 	[TCA_MIRRED_PARMS]	= { .len = sizeof(struct tc_mirred) },
 };
 
-static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
-			   struct tc_action *a, int ovr, int bind)
+static int tcf_mirred_init(struct net *net, struct nlattr *nla,
+			   struct nlattr *est, struct tc_action *a, int ovr,
+			   int bind)
 {
 	struct nlattr *tb[TCA_MIRRED_MAX + 1];
 	struct tc_mirred *parm;

@@ -88,7 +89,7 @@ static int tcf_mirred_init(struct nlattr *nla, struct nlattr *est,
 		return -EINVAL;
 	}
 	if (parm->ifindex) {
-		dev = __dev_get_by_index(&init_net, parm->ifindex);
+		dev = __dev_get_by_index(net, parm->ifindex);
 		if (dev == NULL)
 			return -ENODEV;
 		switch (dev->type) {

net/sched/act_nat.c +1 -1

index b5d029e..876f0ef 100644
@@ -44,7 +44,7 @@ static const struct nla_policy nat_policy[TCA_NAT_MAX + 1] = {
 	[TCA_NAT_PARMS]	= { .len = sizeof(struct tc_nat) },
 };
 
-static int tcf_nat_init(struct nlattr *nla, struct nlattr *est,
+static int tcf_nat_init(struct net *net, struct nlattr *nla, struct nlattr *est,
 			struct tc_action *a, int ovr, int bind)
 {
 	struct nlattr *tb[TCA_NAT_MAX + 1];

net/sched/act_pedit.c +3 -2

index 45c53ab..0c3fadd 100644
@@ -38,8 +38,9 @@ static const struct nla_policy pedit_policy[TCA_PEDIT_MAX + 1] = {
 	[TCA_PEDIT_PARMS]	= { .len = sizeof(struct tc_pedit) },
 };
 
-static int tcf_pedit_init(struct nlattr *nla, struct nlattr *est,
-			  struct tc_action *a, int ovr, int bind)
+static int tcf_pedit_init(struct net *net, struct nlattr *nla,
+			  struct nlattr *est, struct tc_action *a,
+			  int ovr, int bind)
 {
 	struct nlattr *tb[TCA_PEDIT_MAX + 1];
 	struct tc_pedit *parm;

net/sched/act_police.c +3 -2

index a9de232..8dbd695 100644
@@ -130,8 +130,9 @@ static const struct nla_policy police_policy[TCA_POLICE_MAX + 1] = {
 	[TCA_POLICE_RESULT]	= { .type = NLA_U32 },
 };
 
-static int tcf_act_police_locate(struct nlattr *nla, struct nlattr *est,
-				 struct tc_action *a, int ovr, int bind)
+static int tcf_act_police_locate(struct net *net, struct nlattr *nla,
+				 struct nlattr *est, struct tc_action *a,
+				 int ovr, int bind)
 {
 	unsigned int h;
 	int ret = 0, err;

net/sched/act_simple.c +3 -2

index 3714f60..7725eb4 100644
@@ -95,8 +95,9 @@ static const struct nla_policy simple_policy[TCA_DEF_MAX + 1] = {
 	[TCA_DEF_DATA]	= { .type = NLA_STRING, .len = SIMP_MAX_DATA },
 };
 
-static int tcf_simp_init(struct nlattr *nla, struct nlattr *est,
-			 struct tc_action *a, int ovr, int bind)
+static int tcf_simp_init(struct net *net, struct nlattr *nla,
+			 struct nlattr *est, struct tc_action *a,
+			 int ovr, int bind)
 {
 	struct nlattr *tb[TCA_DEF_MAX + 1];
 	struct tc_defact *parm;

net/sched/act_skbedit.c +3 -2

index 476e0fa..cb42211 100644
@@ -67,8 +67,9 @@ static const struct nla_policy skbedit_policy[TCA_SKBEDIT_MAX + 1] = {
 	[TCA_SKBEDIT_MARK]		= { .len = sizeof(u32) },
 };
 
-static int tcf_skbedit_init(struct nlattr *nla, struct nlattr *est,
-			 struct tc_action *a, int ovr, int bind)
+static int tcf_skbedit_init(struct net *net, struct nlattr *nla,
+			    struct nlattr *est, struct tc_action *a,
+			    int ovr, int bind)
 {
 	struct nlattr *tb[TCA_SKBEDIT_MAX + 1];
 	struct tc_skbedit *parm;

net/sched/cls_api.c +6 -5

index ff55ed6..964f5e4 100644
@@ -321,7 +321,7 @@ replay:
 		}
 	}
 
-	err = tp->ops->change(skb, tp, cl, t->tcm_handle, tca, &fh);
+	err = tp->ops->change(net, skb, tp, cl, t->tcm_handle, tca, &fh);
 	if (err == 0) {
 		if (tp_created) {
 			spin_lock_bh(root_lock);

@@ -508,7 +508,7 @@ void tcf_exts_destroy(struct tcf_proto *tp, struct tcf_exts *exts)
 }
 EXPORT_SYMBOL(tcf_exts_destroy);
 
-int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
+int tcf_exts_validate(struct net *net, struct tcf_proto *tp, struct nlattr **tb,
 		  struct nlattr *rate_tlv, struct tcf_exts *exts,
 		  const struct tcf_ext_map *map)
 {

@@ -519,7 +519,7 @@ int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
 		struct tc_action *act;
 
 		if (map->police && tb[map->police]) {
-			act = tcf_action_init_1(tb[map->police], rate_tlv,
+			act = tcf_action_init_1(net, tb[map->police], rate_tlv,
 						"police", TCA_ACT_NOREPLACE,
 						TCA_ACT_BIND);
 			if (IS_ERR(act))

@@ -528,8 +528,9 @@ int tcf_exts_validate(struct tcf_proto *tp, struct nlattr **tb,
 			act->type = TCA_OLD_COMPAT;
 			exts->action = act;
 		} else if (map->action && tb[map->action]) {
-			act = tcf_action_init(tb[map->action], rate_tlv, NULL,
-					      TCA_ACT_NOREPLACE, TCA_ACT_BIND);
+			act = tcf_action_init(net, tb[map->action], rate_tlv,
+					      NULL, TCA_ACT_NOREPLACE,
+					      TCA_ACT_BIND);
 			if (IS_ERR(act))
 				return PTR_ERR(act);

net/sched/cls_basic.c +7 -6

index 344a11b..d76a35d 100644
@@ -132,15 +132,16 @@ static const struct nla_policy basic_policy[TCA_BASIC_MAX + 1] = {
 	[TCA_BASIC_EMATCHES]	= { .type = NLA_NESTED },
 };
 
-static int basic_set_parms(struct tcf_proto *tp, struct basic_filter *f,
-			   unsigned long base, struct nlattr **tb,
+static int basic_set_parms(struct net *net, struct tcf_proto *tp,
+			   struct basic_filter *f, unsigned long base,
+			   struct nlattr **tb,
 			   struct nlattr *est)
 {
 	int err = -EINVAL;
 	struct tcf_exts e;
 	struct tcf_ematch_tree t;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &basic_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &basic_ext_map);
 	if (err < 0)
 		return err;

@@ -162,7 +163,7 @@ errout:
 	return err;
 }
 
-static int basic_change(struct sk_buff *in_skb,
+static int basic_change(struct net *net, struct sk_buff *in_skb,
 			struct tcf_proto *tp, unsigned long base, u32 handle,
 			struct nlattr **tca, unsigned long *arg)
 {

@@ -182,7 +183,7 @@ static int basic_change(struct sk_buff *in_skb,
 	if (f != NULL) {
 		if (handle && f->handle != handle)
 			return -EINVAL;
-		return basic_set_parms(tp, f, base, tb, tca[TCA_RATE]);
+		return basic_set_parms(net, tp, f, base, tb, tca[TCA_RATE]);
 	}
 
 	err = -ENOBUFS;

@@ -208,7 +209,7 @@ static int basic_change(struct sk_buff *in_skb,
 		f->handle = head->hgenerator;
 	}
 
-	err = basic_set_parms(tp, f, base, tb, tca[TCA_RATE]);
+	err = basic_set_parms(net, tp, f, base, tb, tca[TCA_RATE]);
 	if (err < 0)
 		goto errout;

net/sched/cls_cgroup.c +3 -2

index 6db7855..3a294eb 100644
@@ -178,7 +178,7 @@ static const struct nla_policy cgroup_policy[TCA_CGROUP_MAX + 1] = {
 	[TCA_CGROUP_EMATCHES]	= { .type = NLA_NESTED },
 };
 
-static int cls_cgroup_change(struct sk_buff *in_skb,
+static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
 			     struct tcf_proto *tp, unsigned long base,
 			     u32 handle, struct nlattr **tca,
 			     unsigned long *arg)

@@ -215,7 +215,8 @@ static int cls_cgroup_change(struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &cgroup_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e,
+				&cgroup_ext_map);
 	if (err < 0)
 		return err;

net/sched/cls_flow.c +2 -2

index ce82d0c..aa36a8c 100644
@@ -351,7 +351,7 @@ static const struct nla_policy flow_policy[TCA_FLOW_MAX + 1] = {
 	[TCA_FLOW_PERTURB]	= { .type = NLA_U32 },
 };
 
-static int flow_change(struct sk_buff *in_skb, 
+static int flow_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle, struct nlattr **tca,
 		       unsigned long *arg)

@@ -397,7 +397,7 @@ static int flow_change(struct sk_buff *in_skb,
 			return -EOPNOTSUPP;
 	}
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &flow_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &flow_ext_map);
 	if (err < 0)
 		return err;

net/sched/cls_fw.c +5 -5

index 4075a0a..1135d82 100644
@@ -192,7 +192,7 @@ static const struct nla_policy fw_policy[TCA_FW_MAX + 1] = {
 };
 
 static int
-fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
+fw_change_attrs(struct net *net, struct tcf_proto *tp, struct fw_filter *f,
 	struct nlattr **tb, struct nlattr **tca, unsigned long base)
 {
 	struct fw_head *head = (struct fw_head *)tp->root;

@@ -200,7 +200,7 @@ fw_change_attrs(struct tcf_proto *tp, struct fw_filter *f,
 	u32 mask;
 	int err;
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &fw_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &fw_ext_map);
 	if (err < 0)
 		return err;

@@ -233,7 +233,7 @@ errout:
 	return err;
 }
 
-static int fw_change(struct sk_buff *in_skb,
+static int fw_change(struct net *net, struct sk_buff *in_skb,
 		     struct tcf_proto *tp, unsigned long base,
 		     u32 handle,
 		     struct nlattr **tca,

@@ -255,7 +255,7 @@ static int fw_change(struct sk_buff *in_skb,
 	if (f != NULL) {
 		if (f->id != handle && handle)
 			return -EINVAL;
-		return fw_change_attrs(tp, f, tb, tca, base);
+		return fw_change_attrs(net, tp, f, tb, tca, base);
 	}
 
 	if (!handle)

@@ -282,7 +282,7 @@ static int fw_change(struct sk_buff *in_skb,
 
 	f->id = handle;
 
-	err = fw_change_attrs(tp, f, tb, tca, base);
+	err = fw_change_attrs(net, tp, f, tb, tca, base);
 	if (err < 0)
 		goto errout;

net/sched/cls_route.c +8 -7

index c10d57b..37da567 100644
@@ -335,9 +335,10 @@ static const struct nla_policy route4_policy[TCA_ROUTE4_MAX + 1] = {
 	[TCA_ROUTE4_IIF]	= { .type = NLA_U32 },
 };
 
-static int route4_set_parms(struct tcf_proto *tp, unsigned long base,
-	struct route4_filter *f, u32 handle, struct route4_head *head,
-	struct nlattr **tb, struct nlattr *est, int new)
+static int route4_set_parms(struct net *net, struct tcf_proto *tp,
+			    unsigned long base, struct route4_filter *f,
+			    u32 handle, struct route4_head *head,
+			    struct nlattr **tb, struct nlattr *est, int new)
 {
 	int err;
 	u32 id = 0, to = 0, nhandle = 0x8000;

@@ -346,7 +347,7 @@ static int route4_set_parms(struct tcf_proto *tp, unsigned long base,
 	struct route4_bucket *b;
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &route_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &route_ext_map);
 	if (err < 0)
 		return err;

@@ -427,7 +428,7 @@ errout:
 	return err;
 }
 
-static int route4_change(struct sk_buff *in_skb,
+static int route4_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle,
 		       struct nlattr **tca,

@@ -457,7 +458,7 @@ static int route4_change(struct sk_buff *in_skb,
 		if (f->bkt)
 			old_handle = f->handle;
 
-		err = route4_set_parms(tp, base, f, handle, head, tb,
+		err = route4_set_parms(net, tp, base, f, handle, head, tb,
 			tca[TCA_RATE], 0);
 		if (err < 0)
 			return err;

@@ -480,7 +481,7 @@ static int route4_change(struct sk_buff *in_skb,
 	if (f == NULL)
 		goto errout;
 
-	err = route4_set_parms(tp, base, f, handle, head, tb,
+	err = route4_set_parms(net, tp, base, f, handle, head, tb,
 		tca[TCA_RATE], 1);
 	if (err < 0)
 		goto errout;

net/sched/cls_rsvp.h +2 -2

index 494bbb9..252d8b0 100644
@@ -416,7 +416,7 @@ static const struct nla_policy rsvp_policy[TCA_RSVP_MAX + 1] = {
 	[TCA_RSVP_PINFO]	= { .len = sizeof(struct tc_rsvp_pinfo) },
 };
 
-static int rsvp_change(struct sk_buff *in_skb,
+static int rsvp_change(struct net *net, struct sk_buff *in_skb,
 		       struct tcf_proto *tp, unsigned long base,
 		       u32 handle,
 		       struct nlattr **tca,

@@ -440,7 +440,7 @@ static int rsvp_change(struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	err = tcf_exts_validate(tp, tb, tca[TCA_RATE], &e, &rsvp_ext_map);
+	err = tcf_exts_validate(net, tp, tb, tca[TCA_RATE], &e, &rsvp_ext_map);
 	if (err < 0)
 		return err;

net/sched/cls_tcindex.c +8 -6

index a1293b4..b86535a 100644
@@ -197,9 +197,10 @@ static const struct nla_policy tcindex_policy[TCA_TCINDEX_MAX + 1] = {
 };
 
 static int
-tcindex_set_parms(struct tcf_proto *tp, unsigned long base, u32 handle,
-		  struct tcindex_data *p, struct tcindex_filter_result *r,
-		  struct nlattr **tb, struct nlattr *est)
+tcindex_set_parms(struct net *net, struct tcf_proto *tp, unsigned long base,
+		  u32 handle, struct tcindex_data *p,
+		  struct tcindex_filter_result *r, struct nlattr **tb,
+		 struct nlattr *est)
 {
 	int err, balloc = 0;
 	struct tcindex_filter_result new_filter_result, *old_r = r;

@@ -208,7 +209,7 @@ tcindex_set_parms(struct tcf_proto *tp, unsigned long base, u32 handle,
 	struct tcindex_filter *f = NULL; /* make gcc behave */
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &tcindex_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &tcindex_ext_map);
 	if (err < 0)
 		return err;

@@ -332,7 +333,7 @@ errout:
 }
 
 static int
-tcindex_change(struct sk_buff *in_skb,
+tcindex_change(struct net *net, struct sk_buff *in_skb,
 	       struct tcf_proto *tp, unsigned long base, u32 handle,
 	       struct nlattr **tca, unsigned long *arg)
 {

@@ -353,7 +354,8 @@ tcindex_change(struct sk_buff *in_skb,
 	if (err < 0)
 		return err;
 
-	return tcindex_set_parms(tp, base, handle, p, r, tb, tca[TCA_RATE]);
+	return tcindex_set_parms(net, tp, base, handle, p, r, tb,
+				 tca[TCA_RATE]);
 }

net/sched/cls_u32.c +7 -6

index c7c27bc..eb07a1e 100644
@@ -488,15 +488,15 @@ static const struct nla_policy u32_policy[TCA_U32_MAX + 1] = {
 	[TCA_U32_MARK]		= { .len = sizeof(struct tc_u32_mark) },
 };
 
-static int u32_set_parms(struct tcf_proto *tp, unsigned long base,
-			 struct tc_u_hnode *ht,
+static int u32_set_parms(struct net *net, struct tcf_proto *tp,
+			 unsigned long base, struct tc_u_hnode *ht,
 			 struct tc_u_knode *n, struct nlattr **tb,
 			 struct nlattr *est)
 {
 	int err;
 	struct tcf_exts e;
 
-	err = tcf_exts_validate(tp, tb, est, &e, &u32_ext_map);
+	err = tcf_exts_validate(net, tp, tb, est, &e, &u32_ext_map);
 	if (err < 0)
 		return err;

@@ -544,7 +544,7 @@ errout:
 	return err;
 }
 
-static int u32_change(struct sk_buff *in_skb,
+static int u32_change(struct net *net, struct sk_buff *in_skb,
 		      struct tcf_proto *tp, unsigned long base, u32 handle,
 		      struct nlattr **tca,
 		      unsigned long *arg)

@@ -570,7 +570,8 @@ static int u32_change(struct sk_buff *in_skb,
 		if (TC_U32_KEY(n->handle) == 0)
 			return -EINVAL;
 
-		return u32_set_parms(tp, base, n->ht_up, n, tb, tca[TCA_RATE]);
+		return u32_set_parms(net, tp, base, n->ht_up, n, tb,
+				     tca[TCA_RATE]);
 	}
 
 	if (tb[TCA_U32_DIVISOR]) {

@@ -656,7 +657,7 @@ static int u32_change(struct sk_buff *in_skb,
 	}
 #endif
 
-	err = u32_set_parms(tp, base, ht, n, tb, tca[TCA_RATE]);
+	err = u32_set_parms(net, tp, base, ht, n, tb, tca[TCA_RATE]);
 	if (err == 0) {
 		struct tc_u_knode **ins;
 		for (ins = &ht->ht[TC_U32_HASH(handle)]; *ins; ins = &(*ins)->next)

David Miller

maintainer Applied

2013-01-14 20:10 UTC #18 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware act_mirred

From: Benjamin LaHaise <[email protected]>
Date: Mon, 14 Jan 2013 10:15:39 -0500

Quoted 7 lines

Eric Dumazet pointed out that act_mirred needs to find the current net_ns, and struct net pointer is not provided in the call chain. His original patch made use of current->nsproxy->net_ns to find the network namespace, but this fails to work correctly for userspace code that makes use of netlink sockets in different network namespaces. Instead, pass the "struct net *" down along the call chain to where it is needed.

Quoted 6 lines

This version removes the ifb changes as Eric has submitted that patch separately, but is otherwise identical to the previous version.
Signed-off-by Benjamin LaHaise <[email protected]>
Tested-by Eric Dumazet <[email protected]>
Acked-by Jamal Hadi Salim <[email protected]>

Applied, thanks.

David Miller

maintainer Applied

2013-01-14 20:13 UTC #19 lore raw

Re: [PATCH net-next] ifb: dont hard code inet_net use

In reply to Jamal Hadi Salim

From: Jamal Hadi Salim <[email protected]>
Date: Mon, 14 Jan 2013 08:13:45 -0500

Quoted 9 lines · Eric Dumazet

Quoted 5 lines
From: Eric Dumazet <[email protected]>
ifb should lookup devices in the appropriate namespace.
Signed-off-by Eric Dumazet <[email protected]>
Acked-by Jamal Hadi Salim <[email protected]>

Jamal, please don't put leading whitespace in your signoffs and acks, the automated tools that look for these tags in email threads looks for them to start in the first column.

I've applied this patch, thanks.

Thanks.

Benjamin LaHaise

2013-01-13 14:59 UTC #20 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

In reply to Eric Dumazet

Quoted 3 lines · Eric Dumazet

But it is working in my tests.
I added a WARN and the call stack is :

...

Quoted 4 lines

Could you elaborate on what could be the problem ?
We hold the RTNL, so I dont think another process could possibly call tcf_mirred_init()

The locking isn't the issue, but how the network namespace is selected it. I've implemented some virtual router functionality using network namespaces, and prior to having the setns() syscall, the only way to manipulate other network namespaces was via socket passing between threads in different namespaces. One of the optimizations in using this technique was to open a netlink socket in another namespace, then pass that file descriptor back to the main daemon. The code could then add routes and manipulate other areas of the network stack via that netlink socket.

I think this technique is a valid approach for making use of network namespaces. It also has the benefit of avoid the use of setns() for the vast majority of operations.

-ben

Signature · 1 line

"Thought is the essence of where you are now."

Eric Dumazet

2013-01-13 16:35 UTC #21 lore raw

Re: [PATCH net-next] pkt_sched: namespace aware ifb

Quoted 10 lines · Benjamin LaHaise

The locking isn't the issue, but how the network namespace is selected it. I've implemented some virtual router functionality using network namespaces, and prior to having the setns() syscall, the only way to manipulate other network namespaces was via socket passing between threads in different namespaces. One of the optimizations in using this technique was to open a netlink socket in another namespace, then pass that file descriptor back to the main daemon. The code could then add routes and manipulate other areas of the network stack via that netlink socket.

OK thats evil, I'll pass the net pointer then.

Thanks

Oliver Hartkopp

author

2013-01-13 11:01 UTC #22 lore raw

In reply to David Miller

Quoted 5 lines · David Miller

As per your problem with CAN, that's also rediculous. You have an SKB control block in skb->cb[] that you can put whatever values with whatever semantics you want.
Use it.

I'm not writing a RFC to you, when i'm not sure having checked several options before.

In the tx path already net/sched is using cb[] for it's purposes. Adding the information somewhere at the end of cb[] to not interfere with sched becomes tricky. See users of qdisc_cb_private_validate().

So the 'incoming interface index' could be stored safely in the tx path in

- skb->data
- skb_shared_info
- skb_iif

skb_iif was the obvious choice then. And the question why a seven year old if-statement in net/core/dev.c has been removed must be allowed.

Networking is not only IP networking.

Oliver

David Miller

maintainer

2013-01-13 13:20 UTC #23 lore raw

From: Oliver Hartkopp <[email protected]>
Date: Sun, 13 Jan 2013 12:01:53 +0100

Quoted 1 line

Networking is not only IP networking.

But it is who the majority of the infrastructure is goint to be optimized for, and this is never going to change.