Possible regression in __netif_receive_skb() between 2.6.38-rc7 and net-next-2.6

PATCH In review Nicolas de Pesloüan 3 participants 7 messages Mar 5, 2011 – Mar 7, 2011

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Nicolas de Pesloüan

author

2011-03-05 21:30 UTC #0 lore raw

Hi,

Comparing __netif_receive_skb() between 2.6.38-rc7 and net-next-2.6, I noticed an important difference: The ptype_base loop used to deliver to orig_dev and this is not true anymore.

[Note that this is unrelated to Jiri's today's patch that remove the orig_dev parameter to protocol handler].

Imagine the following simple setup:

eth0 -> bond0

- A packet handler registered on eth0, with ptype->type == NULL will receive the packet, because it will be delivered in the ptype_all loop, which is inside the another_round loop.
- The same packet handler, registered on eth0, but with ptype->type != NULL won't receive the packet, because the ptype_base loop doesn't deliver to orig_dev anymore.

I think this can lead to a regression for user space: an application using af_packet to listen to eth0 will receive the packet flow if the registered protocol is NULL, but won't receive anything if the registered protocol is not NULL.

Can someone confirm?

Nicolas.

Jiri Pirko

2011-03-05 22:09 UTC #1 lore raw

Quoted 5 lines · Sat, Mar 05, 2011 at 10:30:33PM CET, [email protected] wrote:

Hi,
Comparing __netif_receive_skb() between 2.6.38-rc7 and net-next-2.6, I noticed an important difference: The ptype_base loop used to deliver to orig_dev and this is not true anymore.

I believe this is adressed by submitted patch " net: allow handlers to be processed for orig_dev"

Quoted 23 lines

[Note that this is unrelated to Jiri's today's patch that remove the orig_dev parameter to protocol handler].
Imagine the following simple setup:
eth0 -> bond0
- A packet handler registered on eth0, with ptype->type == NULL will receive the packet, because it will be delivered in the ptype_all loop, which is inside the another_round loop.
- The same packet handler, registered on eth0, but with ptype->type != NULL won't receive the packet, because the ptype_base loop doesn't deliver to orig_dev anymore.
I think this can lead to a regression for user space: an application using af_packet to listen to eth0 will receive the packet flow if the registered protocol is NULL, but won't receive anything if the registered protocol is not NULL.
Can someone confirm?
Nicolas.

Nicolas de Pesloüan

author

2011-03-06 13:08 UTC #2 lore raw

Quoted 9 lines · Jiri Pirko

Quoted 5 lines · Sat, Mar 05, 2011 at 10:30:33PM CET, [email protected] wrote:
Hi,
Comparing __netif_receive_skb() between 2.6.38-rc7 and net-next-2.6, I noticed an important difference: The ptype_base loop used to deliver to orig_dev and this is not true anymore.
I believe this is adressed by submitted patch " net: allow handlers to be processed for orig_dev"

I wonder whether we should address it that way (which is the former way to address it).

There are still a difference between ptype_all and ptype_base delivery:

- For ptype_all, we deliver to every device crossed while walking the rx_handler path (inside the another_round loop). (And there is no way to force exact match delivery).

- For ptype_base, we deliver to the lowest device (orig_dev) and to the highest (skb->dev) and we can ask for exact match delivery.

This sounds very inconsistent. The only difference between ptype_all and ptype_base is the fact that ptype->type is NULL (wildcard) for the first and not NULL (a particular protocol) for the second.

I think we should:

1/ deliver to both ptype_all and ptype_base while walking the rx_handler path, but only exact match (ptype->dev == skb->dev).
2/ deliver to both ptype_all and ptype_base at the end of __netif_receive_skb(), but only wildcard match (ptype->dev == NULL), skipping this part if the last rx_handler returned RX_HANDLER_EXACT.

Nicolas.

Quoted 20 lines

Quoted 19 lines
Imagine the following simple setup:
eth0 -> bond0
- A packet handler registered on eth0, with ptype->type == NULL will receive the packet, because it will be delivered in the ptype_all loop, which is inside the another_round loop.
- The same packet handler, registered on eth0, but with ptype->type != NULL won't receive the packet, because the ptype_base loop doesn't deliver to orig_dev anymore.
I think this can lead to a regression for user space: an application using af_packet to listen to eth0 will receive the packet flow if the registered protocol is NULL, but won't receive anything if the registered protocol is not NULL.
Can someone confirm?
Nicolas.

Nicolas de Pesloüan

2011-03-06 13:25 UTC #3 lore raw

PATCH 1/1 net: harmonize the call to ptype_all and ptype_base handlers.

Until now, ptype_all and ptype_base delivery in __netif_receive_skb() is inconsistent.

- For ptype_all, we deliver to every device crossed while walking the rx_handler path (inside the another_round loop), and there is no way to stop wildcard delivery (no exact match logic).
- For ptype_base, we deliver to the lowest device (orig_dev) and to the highest (skb->dev) and we can ask for exact match delivery.

This patch try and fix this, by:

1/ Doing exact match delivery for both ptype_all and ptype_base, while walking
the rx_handler path.
2/ Doing wildcard match delivery at the end of __netif_receive_skb(), if not
asked to do exact match delivery only.

Signed-off-by Nicolas de Pesloüan <[email protected]>

---

This apply on top of the last batch of patch from Jiri Pirko.

---
 net/core/dev.c |   32 ++++++++++++++++++++++++--------
 1 files changed, 24 insertions(+), 8 deletions(-)

net/core/dev.c +24 -8

index c71bd18..a368223 100644
@@ -3117,8 +3117,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 {
 	struct packet_type *ptype, *pt_prev;
 	rx_handler_func_t *rx_handler;
-	struct net_device *orig_dev;
-	struct net_device *null_or_dev;
 	bool deliver_exact = false;
 	int ret = NET_RX_DROP;
 	__be16 type;

@@ -3134,7 +3132,6 @@ static int __netif_receive_skb(struct sk_buff *skb)
 
 	if (!skb->skb_iif)
 		skb->skb_iif = skb->dev->ifindex;
-	orig_dev = skb->dev;
 
 	skb_reset_network_header(skb);
 	skb_reset_transport_header(skb);

@@ -3156,7 +3153,17 @@ another_round:
 #endif
 
 	list_for_each_entry_rcu(ptype, &ptype_all, list) {
-		if (!ptype->dev || ptype->dev == skb->dev) {
+		if (ptype->dev == skb->dev) {
+			if (pt_prev)
+				ret = deliver_skb(skb, pt_prev);
+			pt_prev = ptype;
+		}
+	}
+
+	type = skb->protocol;
+	list_for_each_entry_rcu(ptype,
+			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
+		if (ptype->type == type && ptype->dev == skb->dev) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev);
 			pt_prev = ptype;

@@ -3205,20 +3212,29 @@ ncls:
 	vlan_on_bond_hook(skb);
 
 	/* deliver only exact match when indicated */
-	null_or_dev = deliver_exact ? skb->dev : NULL;
+	if (deliver_exact)
+		goto skip_wildcard_delivery;
+
+	list_for_each_entry_rcu(ptype, &ptype_all, list) {
+		if (!ptype->dev) {
+			if (pt_prev)
+				ret = deliver_skb(skb, pt_prev);
+			pt_prev = ptype;
+		}
+	}
 
 	type = skb->protocol;
 	list_for_each_entry_rcu(ptype,
 			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
-		if (ptype->type == type &&
-		    (ptype->dev == null_or_dev || ptype->dev == skb->dev ||
-		     ptype->dev == orig_dev)) {
+		if (ptype->type == type && !ptype->dev) {
 			if (pt_prev)
 				ret = deliver_skb(skb, pt_prev);
 			pt_prev = ptype;
 		}
 	}
 
+skip_wildcard_delivery:	
+
 	if (pt_prev) {
 		ret = pt_prev->func(skb, skb->dev, pt_prev);
 	} else {

Signature · 1 line

1.7.2.3

Jiri Pirko

2011-03-07 10:03 UTC #4 lore raw

Re: [PATCH net-next-2.6] net: harmonize the call to ptype_all and ptype_base handlers.

Quoted 16 lines · Sun, Mar 06, 2011 at 02:25:16PM CET, [email protected] wrote:

Until now, ptype_all and ptype_base delivery in __netif_receive_skb() is inconsistent.
- For ptype_all, we deliver to every device crossed while walking the rx_handler path (inside the another_round loop), and there is no way to stop wildcard delivery (no exact match logic).
- For ptype_base, we deliver to the lowest device (orig_dev) and to the highest (skb->dev) and we can ask for exact match delivery.
This patch try and fix this, by:
1/ Doing exact match delivery for both ptype_all and ptype_base, while walking
the rx_handler path.
2/ Doing wildcard match delivery at the end of __netif_receive_skb(), if not
asked to do exact match delivery only.

Quoted 8 lines

Signed-off-by Nicolas de Pesloüan <[email protected]>
---
This apply on top of the last batch of patch from Jiri Pirko.
---
net/core/dev.c | 32 ++++++++++++++++++++++++--------
1 files changed, 24 insertions(+), 8 deletions(-)

I tend to like this patch. However I'm not sure if extra 2 loops don't introduce noticable overhead :/

Nicolas de Pesloüan

author

2011-03-07 20:41 UTC #5 lore raw

Re: [PATCH net-next-2.6] net: harmonize the call to ptype_all and ptype_base handlers.

Quoted 20 lines · Jiri Pirko

Quoted 19 lines · Sun, Mar 06, 2011 at 02:25:16PM CET, [email protected] wrote:
Until now, ptype_all and ptype_base delivery in __netif_receive_skb() is inconsistent.
- For ptype_all, we deliver to every device crossed while walking the rx_handler path (inside the another_round loop), and there is no way to stop wildcard delivery (no exact match logic).
- For ptype_base, we deliver to the lowest device (orig_dev) and to the highest (skb->dev) and we can ask for exact match delivery.
This patch try and fix this, by:
1/ Doing exact match delivery for both ptype_all and ptype_base, while walking
the rx_handler path.
2/ Doing wildcard match delivery at the end of __netif_receive_skb(), if not
asked to do exact match delivery only.
Signed-off-by Nicolas de Pesloüan<[email protected]>
---

Quoted 8 lines

Quoted 5 lines
This apply on top of the last batch of patch from Jiri Pirko.
---
net/core/dev.c | 32 ++++++++++++++++++++++++--------
1 files changed, 24 insertions(+), 8 deletions(-)
I tend to like this patch. However I'm not sure if extra 2 loops don't introduce noticable overhead :/

I think ptype_all and ptype_base lists should only contain entries having ptype->dev == NULL.

The entries having ptype->dev != NULL should be on per net_device lists. The head of those lists could/should be in a ptype_all and a ptype_base property in net_device.

This would speed up the exact-match loops, because they would scan small (or empty) lists.

I need to double check the possible impact of this proposal.

Nicolas.

Jiri Pirko

2011-03-07 21:12 UTC #6 lore raw

Re: [PATCH net-next-2.6] net: harmonize the call to ptype_all and ptype_base handlers.

Quoted 32 lines · Mon, Mar 07, 2011 at 09:41:19PM CET, [email protected] wrote:

Quoted 20 lines · Jiri Pirko
Quoted 19 lines · Sun, Mar 06, 2011 at 02:25:16PM CET, [email protected] wrote:
Until now, ptype_all and ptype_base delivery in __netif_receive_skb() is inconsistent.
- For ptype_all, we deliver to every device crossed while walking the rx_handler path (inside the another_round loop), and there is no way to stop wildcard delivery (no exact match logic).
- For ptype_base, we deliver to the lowest device (orig_dev) and to the highest (skb->dev) and we can ask for exact match delivery.
This patch try and fix this, by:
1/ Doing exact match delivery for both ptype_all and ptype_base, while walking
the rx_handler path.
2/ Doing wildcard match delivery at the end of __netif_receive_skb(), if not
asked to do exact match delivery only.
Signed-off-by Nicolas de Pesloüan<[email protected]>
---
Quoted 8 lines
Quoted 5 lines
This apply on top of the last batch of patch from Jiri Pirko.
---
net/core/dev.c | 32 ++++++++++++++++++++++++--------
1 files changed, 24 insertions(+), 8 deletions(-)
I tend to like this patch. However I'm not sure if extra 2 loops don't introduce noticable overhead :/
I think ptype_all and ptype_base lists should only contain entries having ptype->dev == NULL.

Quoted 7 lines

The entries having ptype->dev != NULL should be on per net_device lists. The head of those lists could/should be in a ptype_all and a ptype_base property in net_device.
This would speed up the exact-match loops, because they would scan small (or empty) lists.
I need to double check the possible impact of this proposal.

On the first glance, this makes sense to me.

Quoted 2 lines

Nicolas.