[PATCH net] netpoll: linearize skb before accessing its data

[PATCH net] netpoll: linearize skb before accessing its data

[Antonio Quartulli]

__netpoll_rx() assumes that the data buffer of the received
skb is linear and then passes it to rx_hook().
However this is not true because the skb has not been
linearized yet.

This can cause rx_hook() to access non allocated memory
while parsing the received data.

Fix __netpoll_rx() by explicitly linearising the skb.

Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
---

I checked linux-3.0 and this bug seems to be already there. Please consider
queueing it for stable.


Regards,



 net/core/netpoll.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index fc75c9e..97cff18 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -814,6 +814,9 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 		if (pskb_trim_rcsum(skb, len))
 			goto out;
 
+		if (skb_linearize(skb))
+			goto out;
+
 		iph = (struct iphdr *)skb->data;
 		if (iph->protocol != IPPROTO_UDP)
 			goto out;
@@ -855,6 +858,8 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 			goto out;
 		if (pskb_trim_rcsum(skb, len + sizeof(struct ipv6hdr)))
 			goto out;
+		if (skb_linearize(skb))
+			goto out;
 		ip6h = ipv6_hdr(skb);
 		if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 			goto out;
-- 
1.8.4

Re: [PATCH net] netpoll: linearize skb before accessing its data

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Mon, 21 Oct 2013 23:31:20 +0200

> __netpoll_rx() assumes that the data buffer of the received
> skb is linear and then passes it to rx_hook().
> However this is not true because the skb has not been
> linearized yet.
> 
> This can cause rx_hook() to access non allocated memory
> while parsing the received data.
> 
> Fix __netpoll_rx() by explicitly linearising the skb.
> 
> Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>

It is rx_hook's obligation to access the SKB properly and not
assume that the SKB is linear.  It is very expensive to
linearize every SKB just for the sake of improperly implemented
receive hooks.

In particular the rx hooks must make use of interface such
as pskb_may_pull(), just like every other protocol does
on packet input processing, to make sure the area they want
to access is in the linear area.

Re: [PATCH net] netpoll: linearize skb before accessing its data

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1442 bytes --]

On Mon, Oct 21, 2013 at 06:23:19PM -0400, David Miller wrote:
> From: Antonio Quartulli <antonio@meshcoding.com>
> Date: Mon, 21 Oct 2013 23:31:20 +0200
> 
> > __netpoll_rx() assumes that the data buffer of the received
> > skb is linear and then passes it to rx_hook().
> > However this is not true because the skb has not been
> > linearized yet.
> > 
> > This can cause rx_hook() to access non allocated memory
> > while parsing the received data.
> > 
> > Fix __netpoll_rx() by explicitly linearising the skb.
> > 
> > Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
> 
> It is rx_hook's obligation to access the SKB properly and not
> assume that the SKB is linear.  It is very expensive to
> linearize every SKB just for the sake of improperly implemented
> receive hooks.
> 
> In particular the rx hooks must make use of interface such
> as pskb_may_pull(), just like every other protocol does
> on packet input processing, to make sure the area they want
> to access is in the linear area.
> 

But rx_hook() does not receive any skb:

609                 np->rx_hook(np, ntohs(uh->source),
610                                (char *)(uh+1),
611                                ulen - sizeof(struct udphdr));

it just receives a pointer to the data and can't do anything to make it linear.
(uh is a pointer to the udp header). Am I missing something?


Regards,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH net] netpoll: linearize skb before accessing its data

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Tue, 22 Oct 2013 08:06:35 +0200

> But rx_hook() does not receive any skb:
> 
> 609                 np->rx_hook(np, ntohs(uh->source),
> 610                                (char *)(uh+1),
> 611                                ulen - sizeof(struct udphdr));
> 
> it just receives a pointer to the data and can't do anything to make it linear.
> (uh is a pointer to the udp header). Am I missing something?

Then this hook's API needs to be fixed, it's completely broken.

Make it pass the SKB and the appropriate offset (from skb->data) in
bytes.

Re: [PATCH net] netpoll: linearize skb before accessing its data

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1014 bytes --]

On Tue, Oct 22, 2013 at 02:25:25AM -0400, David Miller wrote:
> From: Antonio Quartulli <antonio@meshcoding.com>
> Date: Tue, 22 Oct 2013 08:06:35 +0200
> 
> > But rx_hook() does not receive any skb:
> > 
> > 609                 np->rx_hook(np, ntohs(uh->source),
> > 610                                (char *)(uh+1),
> > 611                                ulen - sizeof(struct udphdr));
> > 
> > it just receives a pointer to the data and can't do anything to make it linear.
> > (uh is a pointer to the udp header). Am I missing something?
> 
> Then this hook's API needs to be fixed, it's completely broken.
> 
> Make it pass the SKB and the appropriate offset (from skb->data) in
> bytes.
> 

Ok I will do that. But I think this API change must go into net-next, right?
Otherwise we would break every existing user.

What about resending this patch to net (after having removed the pskb_may_pull()
as pointed out by Eric) and then fixing the API in net-next?

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH net] netpoll: linearize skb before accessing its data

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Tue, 22 Oct 2013 08:37:14 +0200

> Ok I will do that. But I think this API change must go into
> net-next, right?  Otherwise we would break every existing user.

Too bad, it's a necessary API change to fix the bug, the users
will need to change too.

It's not our problem if they are out-of-tree and can't be fixed
in-situ.

[PATCH net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

Right now skb->data is passed to rx_hook() even if the skb
has not been linearised and without giving rx_hook() a way
to linearise it.

Change the rx_hook() interface and make it accept the skb
as argument. In this way users implementing rx_hook() can
perform all the needed operations to properly (and safely)
access the skb data.

Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
---
 include/linux/netpoll.h |  2 +-
 net/core/netpoll.c      | 10 ++++------
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index f3c7c24..5352160 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -24,7 +24,7 @@ struct netpoll {
 	struct net_device *dev;
 	char dev_name[IFNAMSIZ];
 	const char *name;
-	void (*rx_hook)(struct netpoll *, int, char *, int);
+	void (*rx_hook)(struct netpoll *np, struct sk_buff *skb, int offset);
 
 	union inet_addr local_ip, remote_ip;
 	bool ipv6;
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index fc75c9e..b415437 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -834,9 +834,8 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 			if (np->local_port && np->local_port != ntohs(uh->dest))
 				continue;
 
-			np->rx_hook(np, ntohs(uh->source),
-				       (char *)(uh+1),
-				       ulen - sizeof(struct udphdr));
+			np->rx_hook(np, skb,
+				    (unsigned char *)(uh + 1) - skb->data);
 			hits++;
 		}
 	} else {
@@ -872,9 +871,8 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 			if (np->local_port && np->local_port != ntohs(uh->dest))
 				continue;
 
-			np->rx_hook(np, ntohs(uh->source),
-				       (char *)(uh+1),
-				       ulen - sizeof(struct udphdr));
+			np->rx_hook(np, skb,
+				    (unsigned char *)(uh + 1) - skb->data);
 			hits++;
 		}
 #endif
-- 
1.8.4

RE: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[David Laight]

> Subject: [PATCH net] netpoll: fix rx_hook() interface by passing the skb
> 
> Right now skb->data is passed to rx_hook() even if the skb
> has not been linearised and without giving rx_hook() a way
> to linearise it.
> 
> Change the rx_hook() interface and make it accept the skb
> as argument. In this way users implementing rx_hook() can
> perform all the needed operations to properly (and safely)
> access the skb data.
...
> -	void (*rx_hook)(struct netpoll *, int, char *, int);
> +	void (*rx_hook)(struct netpoll *np, struct sk_buff *skb, int offset);

You can't do that change without changing the way that hooks are registered
so that any existing modules will fail to register their hooks.

	David

Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Tue, Oct 22, 2013 at 10:09:00AM +0100, David Laight wrote:
> > Subject: [PATCH net] netpoll: fix rx_hook() interface by passing the skb
> > 
> > Right now skb->data is passed to rx_hook() even if the skb
> > has not been linearised and without giving rx_hook() a way
> > to linearise it.
> > 
> > Change the rx_hook() interface and make it accept the skb
> > as argument. In this way users implementing rx_hook() can
> > perform all the needed operations to properly (and safely)
> > access the skb data.
> ...
> > -	void (*rx_hook)(struct netpoll *, int, char *, int);
> > +	void (*rx_hook)(struct netpoll *np, struct sk_buff *skb, int offset);
> 
> You can't do that change without changing the way that hooks are registered
> so that any existing modules will fail to register their hooks.

There is no hook registration in the kernel tree. All the users are outside.


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[David Laight]

> Subject: Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb
> 
> On Tue, Oct 22, 2013 at 10:09:00AM +0100, David Laight wrote:
> > > Subject: [PATCH net] netpoll: fix rx_hook() interface by passing the skb
> > >
> > > Right now skb->data is passed to rx_hook() even if the skb
> > > has not been linearised and without giving rx_hook() a way
> > > to linearise it.
> > >
> > > Change the rx_hook() interface and make it accept the skb
> > > as argument. In this way users implementing rx_hook() can
> > > perform all the needed operations to properly (and safely)
> > > access the skb data.
> > ...
> > > -	void (*rx_hook)(struct netpoll *, int, char *, int);
> > > +	void (*rx_hook)(struct netpoll *np, struct sk_buff *skb, int offset);
> >
> > You can't do that change without changing the way that hooks are registered
> > so that any existing modules will fail to register their hooks.
> 
> There is no hook registration in the kernel tree. All the users are outside.

Looking at __netpoll_rx() I notice that there isn't an skb_pull for the
udp header.

Actually, I think the alignment rules effectively imply that iph->ihl
(the second byte) will always be in the first skb fragment so the
code could sensible do a single skb_pull() that includes the udp header.

I can't remember which value you passed as 'offset' (and my mailer makes
it hard to find), but to ease the code changes the offset of the udp data
would make sense.
In that case you still need to pass the source port.
If you do rx_hook(np, source_port, skb, offset) then if anyone manages to
load an old module (or code that casts the assignement to rx_poll)
at least it won't go 'bang'.
Renaming the structure member will guarantee to generate compile errors.

	David

Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Tue, Oct 22, 2013 at 01:46:22PM +0100, David Laight wrote:
> > Subject: Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb
> > 
> > On Tue, Oct 22, 2013 at 10:09:00AM +0100, David Laight wrote:
> > > > Subject: [PATCH net] netpoll: fix rx_hook() interface by passing the skb
> > > >
> > > > Right now skb->data is passed to rx_hook() even if the skb
> > > > has not been linearised and without giving rx_hook() a way
> > > > to linearise it.
> > > >
> > > > Change the rx_hook() interface and make it accept the skb
> > > > as argument. In this way users implementing rx_hook() can
> > > > perform all the needed operations to properly (and safely)
> > > > access the skb data.
> > > ...
> > > > -	void (*rx_hook)(struct netpoll *, int, char *, int);
> > > > +	void (*rx_hook)(struct netpoll *np, struct sk_buff *skb, int offset);
> > >
> > > You can't do that change without changing the way that hooks are registered
> > > so that any existing modules will fail to register their hooks.
> > 
> > There is no hook registration in the kernel tree. All the users are outside.
> 
> Looking at __netpoll_rx() I notice that there isn't an skb_pull for the
> udp header.
> 
> Actually, I think the alignment rules effectively imply that iph->ihl
> (the second byte) will always be in the first skb fragment so the
> code could sensible do a single skb_pull() that includes the udp header.
> 
> I can't remember which value you passed as 'offset' (and my mailer makes
> it hard to find), but to ease the code changes the offset of the udp data
> would make sense.
> In that case you still need to pass the source port.

I decided not to pass the source port because if the user is really interested
in it, it is still possible to get the udp_hdr from the skb and read its value.

> If you do rx_hook(np, source_port, skb, offset) then if anyone manages to
> load an old module (or code that casts the assignement to rx_poll)
> at least it won't go 'bang'.
> Renaming the structure member will guarantee to generate compile errors.
> 

so you suggest to rename rx_hook to something else to warn people about the
change?

If we go for the "no udp port" approach they will get an error any way because
of the mismatching arguments.

Regards,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Tue, 22 Oct 2013 19:13:14 +0200

> If we go for the "no udp port" approach they will get an error any
> way because of the mismatching arguments.

Antonio, I think it is fair enough to keep passing the port argument
as well as the length.

These precomputed values might as well be provided the to receiver.

Thanks.

RE: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[David Laight]

...
> > I can't remember which value you passed as 'offset' (and my mailer makes
> > it hard to find), but to ease the code changes the offset of the udp data
> > would make sense.
> > In that case you still need to pass the source port.
> 
> I decided not to pass the source port because if the user is really interested
> in it, it is still possible to get the udp_hdr from the skb and read its value.

It just seemed that there was no need to require that the hook re-parse
the ip header just to find the source port.
(ok it could assume that the udp header is just before the data)
 
> > If you do rx_hook(np, source_port, skb, offset) then if anyone manages to
> > load an old module (or code that casts the assignement to rx_poll)
> > at least it won't go 'bang'.
> > Renaming the structure member will guarantee to generate compile errors.
> 
> so you suggest to rename rx_hook to something else to warn people about the
> change?

Yes.

> If we go for the "no udp port" approach they will get an error any way because
> of the mismatching arguments.

No - you only get a warning when you assign a function pointer of the wrong type.
And that is true even if you just change the type of the pointer.
However code might already have a cast on the function pointer (eg because the
hook has 'unsigned char *') - so you won't even get a warning.
You then get an OOPS when the hook tries to read the buffer.

It is a really bad interface...
There isn't even a flags/options (etc) word that can be used
to detect enhancements.

	David

Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 2175 bytes --]

On Wed, Oct 23, 2013 at 09:33:49AM +0100, David Laight wrote:
> ...
> > > I can't remember which value you passed as 'offset' (and my mailer makes
> > > it hard to find), but to ease the code changes the offset of the udp data
> > > would make sense.
> > > In that case you still need to pass the source port.
> > 
> > I decided not to pass the source port because if the user is really interested
> > in it, it is still possible to get the udp_hdr from the skb and read its value.
> 
> It just seemed that there was no need to require that the hook re-parse
> the ip header just to find the source port.
> (ok it could assume that the udp header is just before the data)

Also David (M.) pointed out the same. I will keep the port as argument for
rx_hook.

>  
> > > If you do rx_hook(np, source_port, skb, offset) then if anyone manages to
> > > load an old module (or code that casts the assignement to rx_poll)
> > > at least it won't go 'bang'.
> > > Renaming the structure member will guarantee to generate compile errors.
> > 
> > so you suggest to rename rx_hook to something else to warn people about the
> > change?
> 
> Yes.
> 

mh..what about rx_skb_hook ? this way we also make it easy to notice the
difference (both in arguments and behaviour).

> > If we go for the "no udp port" approach they will get an error any way because
> > of the mismatching arguments.
> 
> No - you only get a warning when you assign a function pointer of the wrong type.
> And that is true even if you just change the type of the pointer.
> However code might already have a cast on the function pointer (eg because the
> hook has 'unsigned char *') - so you won't even get a warning.
> You then get an OOPS when the hook tries to read the buffer.
> 
> It is a really bad interface...
> There isn't even a flags/options (etc) word that can be used
> to detect enhancements.
> 


agreed. But I am not sure about what I could do to fix that.

My idea is to use the following API:

rx_skb_hook(struct netpoll *np, int source, struct sk_buff *skb, int len);


Any suggestion or objection?


Regards,


-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[David Laight]

> My idea is to use the following API:
> 
> rx_skb_hook(struct netpoll *np, int source, struct sk_buff *skb, int len);
> 
> Any suggestion or objection?

Don't you need to pass the offset of the udp data?

	David

Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Wed, Oct 23, 2013 at 12:18:32PM +0100, David Laight wrote:
> > My idea is to use the following API:
> > 
> > rx_skb_hook(struct netpoll *np, int source, struct sk_buff *skb, int len);
> > 
> > Any suggestion or objection?
> 
> Don't you need to pass the offset of the udp data?

Yes, you are right. I just forgot it. Therefore we have:

rx_skb_hook(struct netpoll *np, int source, struct sk_buff *skb, int offset,
	    int len);

where offset is going to be = (udp_hdr + 1) - skb->data
and len = skb->len - offset

Regards,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH net] netpoll: fix rx_hook() interface by passing the skb

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Wed, 23 Oct 2013 14:44:01 +0200

> On Wed, Oct 23, 2013 at 12:18:32PM +0100, David Laight wrote:
>> > My idea is to use the following API:
>> > 
>> > rx_skb_hook(struct netpoll *np, int source, struct sk_buff *skb, int len);
>> > 
>> > Any suggestion or objection?
>> 
>> Don't you need to pass the offset of the udp data?
> 
> Yes, you are right. I just forgot it. Therefore we have:
> 
> rx_skb_hook(struct netpoll *np, int source, struct sk_buff *skb, int offset,
> 	    int len);
> 
> where offset is going to be = (udp_hdr + 1) - skb->data
> and len = skb->len - offset

This looks good to me.

[PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

Right now skb->data is passed to rx_hook() even if the skb
has not been linearised and without giving rx_hook() a way
to linearise it.

Change the rx_hook() interface and make it accept the skb
and the offset to the UDP payload as arguments. rx_hook() is
also renamed to rx_skb_hook() to ensure that out of the tree
users notice the API change.

In this way any rx_skb_hook() implementation can perform all
the needed operations to properly (and safely) access the
skb data.

Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
---
 include/linux/netpoll.h |  5 +++--
 net/core/netpoll.c      | 31 ++++++++++++++++++-------------
 2 files changed, 21 insertions(+), 15 deletions(-)

diff --git a/include/linux/netpoll.h b/include/linux/netpoll.h
index f3c7c24..fbfdb9d 100644
--- a/include/linux/netpoll.h
+++ b/include/linux/netpoll.h
@@ -24,7 +24,8 @@ struct netpoll {
 	struct net_device *dev;
 	char dev_name[IFNAMSIZ];
 	const char *name;
-	void (*rx_hook)(struct netpoll *, int, char *, int);
+	void (*rx_skb_hook)(struct netpoll *np, int source, struct sk_buff *skb,
+			    int offset, int len);
 
 	union inet_addr local_ip, remote_ip;
 	bool ipv6;
@@ -41,7 +42,7 @@ struct netpoll_info {
 	unsigned long rx_flags;
 	spinlock_t rx_lock;
 	struct semaphore dev_lock;
-	struct list_head rx_np; /* netpolls that registered an rx_hook */
+	struct list_head rx_np; /* netpolls that registered an rx_skb_hook */
 
 	struct sk_buff_head neigh_tx; /* list of neigh requests to reply to */
 	struct sk_buff_head txq;
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index fc75c9e..8f97199 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -636,8 +636,9 @@ static void netpoll_neigh_reply(struct sk_buff *skb, struct netpoll_info *npinfo
 
 			netpoll_send_skb(np, send_skb);
 
-			/* If there are several rx_hooks for the same address,
-			   we're fine by sending a single reply */
+			/* If there are several rx_skb_hooks for the same
+			 * address we're fine by sending a single reply
+			 */
 			break;
 		}
 		spin_unlock_irqrestore(&npinfo->rx_lock, flags);
@@ -719,8 +720,9 @@ static void netpoll_neigh_reply(struct sk_buff *skb, struct netpoll_info *npinfo
 
 			netpoll_send_skb(np, send_skb);
 
-			/* If there are several rx_hooks for the same address,
-			   we're fine by sending a single reply */
+			/* If there are several rx_skb_hooks for the same
+			 * address, we're fine by sending a single reply
+			 */
 			break;
 		}
 		spin_unlock_irqrestore(&npinfo->rx_lock, flags);
@@ -756,11 +758,12 @@ static bool pkt_is_ns(struct sk_buff *skb)
 
 int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 {
-	int proto, len, ulen;
-	int hits = 0;
+	int proto, len, ulen, data_len;
+	int hits = 0, offset;
 	const struct iphdr *iph;
 	struct udphdr *uh;
 	struct netpoll *np, *tmp;
+	uint16_t source;
 
 	if (list_empty(&npinfo->rx_np))
 		goto out;
@@ -820,7 +823,10 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 
 		len -= iph->ihl*4;
 		uh = (struct udphdr *)(((char *)iph) + iph->ihl*4);
+		offset = (unsigned char *)(uh + 1) - skb->data;
 		ulen = ntohs(uh->len);
+		data_len = skb->len - offset;
+		source = ntohs(uh->source);
 
 		if (ulen != len)
 			goto out;
@@ -834,9 +840,7 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 			if (np->local_port && np->local_port != ntohs(uh->dest))
 				continue;
 
-			np->rx_hook(np, ntohs(uh->source),
-				       (char *)(uh+1),
-				       ulen - sizeof(struct udphdr));
+			np->rx_skb_hook(np, source, skb, offset, data_len);
 			hits++;
 		}
 	} else {
@@ -859,7 +863,10 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 		if (!pskb_may_pull(skb, sizeof(struct udphdr)))
 			goto out;
 		uh = udp_hdr(skb);
+		offset = (unsigned char *)(uh + 1) - skb->data;
 		ulen = ntohs(uh->len);
+		data_len = skb->len - offset;
+		source = ntohs(uh->source);
 		if (ulen != skb->len)
 			goto out;
 		if (udp6_csum_init(skb, uh, IPPROTO_UDP))
@@ -872,9 +879,7 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
 			if (np->local_port && np->local_port != ntohs(uh->dest))
 				continue;
 
-			np->rx_hook(np, ntohs(uh->source),
-				       (char *)(uh+1),
-				       ulen - sizeof(struct udphdr));
+			np->rx_skb_hook(np, source, skb, offset, data_len);
 			hits++;
 		}
 #endif
@@ -1062,7 +1067,7 @@ int __netpoll_setup(struct netpoll *np, struct net_device *ndev, gfp_t gfp)
 
 	npinfo->netpoll = np;
 
-	if (np->rx_hook) {
+	if (np->rx_skb_hook) {
 		spin_lock_irqsave(&npinfo->rx_lock, flags);
 		npinfo->rx_flags |= NETPOLL_RX_ENABLED;
 		list_add_tail(&np->rx, &npinfo->rx_np);
-- 
1.8.4

RE: [PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb

[David Laight]

> Subject: [PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb
> 
> Right now skb->data is passed to rx_hook() even if the skb
> has not been linearised and without giving rx_hook() a way
> to linearise it.
> 
> Change the rx_hook() interface and make it accept the skb
> and the offset to the UDP payload as arguments. rx_hook() is
> also renamed to rx_skb_hook() to ensure that out of the tree
> users notice the API change.
> 
> In this way any rx_skb_hook() implementation can perform all
> the needed operations to properly (and safely) access the
> skb data.
...
> @@ -820,7 +823,10 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
> 
>  		len -= iph->ihl*4;
>  		uh = (struct udphdr *)(((char *)iph) + iph->ihl*4);
> +		offset = (unsigned char *)(uh + 1) - skb->data;
>  		ulen = ntohs(uh->len);
> +		data_len = skb->len - offset;
> +		source = ntohs(uh->source);
> 
>  		if (ulen != len)
>  			goto out;
> @@ -834,9 +840,7 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
>  			if (np->local_port && np->local_port != ntohs(uh->dest))
>  				continue;
> 
> -			np->rx_hook(np, ntohs(uh->source),
> -				       (char *)(uh+1),
> -				       ulen - sizeof(struct udphdr));
> +			np->rx_skb_hook(np, source, skb, offset, data_len);
>  			hits++;
>  		}
>  	} else {

>From a code optimisation point of view you probably don't want to be
calculating the source, offset and length early.
It is quite likely that the local variables will have to be written
to the stack (because of the function calls) - so it is almost
certainly more efficient to calculate them just before the call.

	David

Re: [PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]

On Thu, Oct 24, 2013 at 09:43:38AM +0100, David Laight wrote:
> > Subject: [PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb
> > @@ -820,7 +823,10 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
> > 
> >  		len -= iph->ihl*4;
> >  		uh = (struct udphdr *)(((char *)iph) + iph->ihl*4);
> > +		offset = (unsigned char *)(uh + 1) - skb->data;
> >  		ulen = ntohs(uh->len);
> > +		data_len = skb->len - offset;
> > +		source = ntohs(uh->source);
> > 
> >  		if (ulen != len)
> >  			goto out;
> > @@ -834,9 +840,7 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
> >  			if (np->local_port && np->local_port != ntohs(uh->dest))
> >  				continue;
> > 
> > -			np->rx_hook(np, ntohs(uh->source),
> > -				       (char *)(uh+1),
> > -				       ulen - sizeof(struct udphdr));
> > +			np->rx_skb_hook(np, source, skb, offset, data_len);
> >  			hits++;
> >  		}
> >  	} else {
> 
> From a code optimisation point of view you probably don't want to be
> calculating the source, offset and length early.
> It is quite likely that the local variables will have to be written
> to the stack (because of the function calls) - so it is almost
> certainly more efficient to calculate them just before the call.

I thought that computing them once outside the loop was better than
re-computing them during each iteration.

Having them outside makes it also clear that they always have the same value.


Regards,

-- 
Antonio Quartulli

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb

[David Miller]

From: "David Laight" <David.Laight@ACULAB.COM>
Date: Thu, 24 Oct 2013 09:43:38 +0100

> From a code optimisation point of view you probably don't want to be
> calculating the source, offset and length early.
> It is quite likely that the local variables will have to be written
> to the stack (because of the function calls) - so it is almost
> certainly more efficient to calculate them just before the call.

But this change is being made from a bug prevention point of view,
we already we're already getting this code path wrong as-is.

Re: [PATCHv2 net] netpoll: fix rx_hook() interface by passing the skb

[David Miller]

From: Antonio Quartulli <antonio@meshcoding.com>
Date: Wed, 23 Oct 2013 23:36:30 +0200

> Right now skb->data is passed to rx_hook() even if the skb
> has not been linearised and without giving rx_hook() a way
> to linearise it.
> 
> Change the rx_hook() interface and make it accept the skb
> and the offset to the UDP payload as arguments. rx_hook() is
> also renamed to rx_skb_hook() to ensure that out of the tree
> users notice the API change.
> 
> In this way any rx_skb_hook() implementation can perform all
> the needed operations to properly (and safely) access the
> skb data.
> 
> Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>

Applied.

Re: [PATCH net] netpoll: linearize skb before accessing its data

[Eric Dumazet]

On Mon, 2013-10-21 at 23:31 +0200, Antonio Quartulli wrote:
> __netpoll_rx() assumes that the data buffer of the received
> skb is linear and then passes it to rx_hook().
> However this is not true because the skb has not been
> linearized yet.
> 
> This can cause rx_hook() to access non allocated memory
> while parsing the received data.
> 
> Fix __netpoll_rx() by explicitly linearising the skb.
> 
> Signed-off-by: Antonio Quartulli <antonio@meshcoding.com>
> ---
> 
> I checked linux-3.0 and this bug seems to be already there. Please consider
> queueing it for stable.
> 
> 
> Regards,
> 
> 
> 
>  net/core/netpoll.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index fc75c9e..97cff18 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -814,6 +814,9 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
>  		if (pskb_trim_rcsum(skb, len))
>  			goto out;
>  
> +		if (skb_linearize(skb))
> +			goto out;
> +
>  		iph = (struct iphdr *)skb->data;
>  		if (iph->protocol != IPPROTO_UDP)
>  			goto out;
> @@ -855,6 +858,8 @@ int __netpoll_rx(struct sk_buff *skb, struct netpoll_info *npinfo)
>  			goto out;
>  		if (pskb_trim_rcsum(skb, len + sizeof(struct ipv6hdr)))
>  			goto out;
> +		if (skb_linearize(skb))
> +			goto out;
>  		ip6h = ipv6_hdr(skb);
>  		if (!pskb_may_pull(skb, sizeof(struct udphdr)))
>  			goto out;

Well, if you linearize the skb, no need for pskb_may_pull(),
and it would be better to do it once at the beginning...


Anyway, how I see nothing sets rx_hook, what am I missing ?

# git grep -n rx_hook
include/linux/netpoll.h:27:     void (*rx_hook)(struct netpoll *, int, char *, int);
include/linux/netpoll.h:44:     struct list_head rx_np; /* netpolls that registered an rx_hook */
net/core/netpoll.c:639:                 /* If there are several rx_hooks for the same address,
net/core/netpoll.c:722:                 /* If there are several rx_hooks for the same address,
net/core/netpoll.c:837:                 np->rx_hook(np, ntohs(uh->source),
net/core/netpoll.c:875:                 np->rx_hook(np, ntohs(uh->source),
net/core/netpoll.c:1065:        if (np->rx_hook) {

Re: [PATCH net] netpoll: linearize skb before accessing its data

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 21 Oct 2013 15:25:36 -0700

> Anyway, how I see nothing sets rx_hook, what am I missing ?

Only out of tree code makes use of this facility, and it's been
this way since the facility was introduced.

Yes, I'm disappointed and unhappy about this too.