problem with 'net: Partially allow skb destructors to be used on receive path'

problem with 'net: Partially allow skb destructors to be used on receive path'

[Oliver Hartkopp]

Hello Herbert,

i got a feedback on the SocketCAN users ML from Michel Marti that the can-raw
socket option CAN_RAW_RECV_OWN_MSGS is out of order since 2.6.30.

https://lists.berlios.de/pipermail/socketcan-users/2009-June/000959.html

Usually the user application does not get CAN frames back through the socket
it was originally sent:

static void raw_rcv(struct sk_buff *skb, void *data)
{
        struct sock *sk = (struct sock *)data;
        struct raw_sock *ro = raw_sk(sk);
        struct sockaddr_can *addr;

        /* check the received tx sock reference */
        if (!ro->recv_own_msgs && skb->sk == sk)
                return;

...

For another detail see can_send() at net/can/af_can.c

The needed sk reference in the rx path for omitting the own received messages
is killed by a new skb_orphan() call in net/core/dev.c introduced in 2.6.30:

"net: Partially allow skb destructors to be used on receive path"

http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.30.y.git;a=commitdiff;h=9a279bcbe347496799711155ed41a89bc40f79c5

I tried to follow your comment of the commit which looks mostly associated to
IP networking.

Do you have any idea how this currently killed functionality that bases on the
originator skb->sk could be fixed?

Suggestions:

1. omit the skb_orphan() for ARPHRD_CAN skbs
2. put the sk value in cb[]
3. anything else ???

Thanks,
Oliver

Re: problem with 'net: Partially allow skb destructors to be used on receive path'

[Herbert Xu]

On Sat, Jun 20, 2009 at 03:17:36PM +0200, Oliver Hartkopp wrote:
> Hello Herbert,
> 
> i got a feedback on the SocketCAN users ML from Michel Marti that the can-raw
> socket option CAN_RAW_RECV_OWN_MSGS is out of order since 2.6.30.
> 
> https://lists.berlios.de/pipermail/socketcan-users/2009-June/000959.html

OK this patch should fix the problem.

net: Move rx skb_orphan call to where needed

In order to get the tun driver to account packets, we need to be
able to receive packets with destructors set.  To be on the safe
side, I added an skb_orphan call for all protocols by default since
some of them (IP in particular) cannot handle receiving packets
destructors properly.

Now it seems that at least one protocol (CAN) expects to be able
to pass skb->sk through the rx path without getting clobbered.

So this patch attempts to fix this properly by moving the skb_orphan
call to where it's actually needed.  In particular, I've added it
to skb_set_owner_[rw] which is what most users of skb->destructor
call.

This is actually an improvement for tun too since it means that
we only give back the amount charged to the socket when the skb
is passed to another socket that will also be charged accordingly.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
index 9f80a76..d16a304 100644
--- a/include/net/sctp/sctp.h
+++ b/include/net/sctp/sctp.h
@@ -448,6 +448,7 @@ static inline void sctp_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
 	struct sctp_ulpevent *event = sctp_skb2event(skb);
 
+	skb_orphan(skb);
 	skb->sk = sk;
 	skb->destructor = sctp_sock_rfree;
 	atomic_add(event->rmem_len, &sk->sk_rmem_alloc);
diff --git a/include/net/sock.h b/include/net/sock.h
index 07133c5..352f06b 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1252,6 +1252,7 @@ static inline int sk_has_allocations(const struct sock *sk)
 
 static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
 {
+	skb_orphan(skb);
 	skb->sk = sk;
 	skb->destructor = sock_wfree;
 	/*
@@ -1264,6 +1265,7 @@ static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
 
 static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
+	skb_orphan(skb);
 	skb->sk = sk;
 	skb->destructor = sock_rfree;
 	atomic_add(skb->truesize, &sk->sk_rmem_alloc);
diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
index 5f1d210..de56d39 100644
--- a/net/ax25/ax25_in.c
+++ b/net/ax25/ax25_in.c
@@ -437,8 +437,7 @@ free:
 int ax25_kiss_rcv(struct sk_buff *skb, struct net_device *dev,
 		  struct packet_type *ptype, struct net_device *orig_dev)
 {
-	skb->sk = NULL;		/* Initially we don't know who it's for */
-	skb->destructor = NULL;	/* Who initializes this, dammit?! */
+	skb_orphan(skb);
 
 	if (!net_eq(dev_net(dev), &init_net)) {
 		kfree_skb(skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index baf2dc1..60b5728 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2310,8 +2310,6 @@ ncls:
 	if (!skb)
 		goto out;
 
-	skb_orphan(skb);
-
 	type = skb->protocol;
 	list_for_each_entry_rcu(ptype,
 			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
index 5922feb..cb762c8 100644
--- a/net/irda/af_irda.c
+++ b/net/irda/af_irda.c
@@ -913,9 +913,6 @@ static int irda_accept(struct socket *sock, struct socket *newsock, int flags)
 	/* Clean up the original one to keep it in listen state */
 	irttp_listen(self->tsap);
 
-	/* Wow ! What is that ? Jean II */
-	skb->sk = NULL;
-	skb->destructor = NULL;
 	kfree_skb(skb);
 	sk->sk_ack_backlog--;
 
diff --git a/net/irda/ircomm/ircomm_lmp.c b/net/irda/ircomm/ircomm_lmp.c
index 67c99d2..7ba9661 100644
--- a/net/irda/ircomm/ircomm_lmp.c
+++ b/net/irda/ircomm/ircomm_lmp.c
@@ -196,6 +196,7 @@ static int ircomm_lmp_data_request(struct ircomm_cb *self,
 	/* Don't forget to refcount it - see ircomm_tty_do_softint() */
 	skb_get(skb);
 
+	skb_orphan(skb);
 	skb->destructor = ircomm_lmp_flow_control;
 
 	if ((self->pkt_count++ > 7) && (self->flow_status == FLOW_START)) {

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: problem with 'net: Partially allow skb destructors to be used on receive path'

[Oliver Hartkopp]

Herbert Xu wrote:
> On Sat, Jun 20, 2009 at 03:17:36PM +0200, Oliver Hartkopp wrote:
>> Hello Herbert,
>>
>> i got a feedback on the SocketCAN users ML from Michel Marti that the can-raw
>> socket option CAN_RAW_RECV_OWN_MSGS is out of order since 2.6.30.
>>
>> https://lists.berlios.de/pipermail/socketcan-users/2009-June/000959.html
> 
> OK this patch should fix the problem.

It does - at least for me ;-)

I tried to apply the patch on 2.6.30 also and only got some offsets (as expected).

Do you think, it's a good idea to queue this up for 2.6.30-stable?

Many thanks,
Oliver

Tested-by: Oliver Hartkopp <olver@hartkopp.net>

> 
> net: Move rx skb_orphan call to where needed
> 
> In order to get the tun driver to account packets, we need to be
> able to receive packets with destructors set.  To be on the safe
> side, I added an skb_orphan call for all protocols by default since
> some of them (IP in particular) cannot handle receiving packets
> destructors properly.
> 
> Now it seems that at least one protocol (CAN) expects to be able
> to pass skb->sk through the rx path without getting clobbered.
> 
> So this patch attempts to fix this properly by moving the skb_orphan
> call to where it's actually needed.  In particular, I've added it
> to skb_set_owner_[rw] which is what most users of skb->destructor
> call.
> 
> This is actually an improvement for tun too since it means that
> we only give back the amount charged to the socket when the skb
> is passed to another socket that will also be charged accordingly.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 9f80a76..d16a304 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -448,6 +448,7 @@ static inline void sctp_skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
>  {
>  	struct sctp_ulpevent *event = sctp_skb2event(skb);
>  
> +	skb_orphan(skb);
>  	skb->sk = sk;
>  	skb->destructor = sctp_sock_rfree;
>  	atomic_add(event->rmem_len, &sk->sk_rmem_alloc);
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 07133c5..352f06b 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -1252,6 +1252,7 @@ static inline int sk_has_allocations(const struct sock *sk)
>  
>  static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
>  {
> +	skb_orphan(skb);
>  	skb->sk = sk;
>  	skb->destructor = sock_wfree;
>  	/*
> @@ -1264,6 +1265,7 @@ static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
>  
>  static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
>  {
> +	skb_orphan(skb);
>  	skb->sk = sk;
>  	skb->destructor = sock_rfree;
>  	atomic_add(skb->truesize, &sk->sk_rmem_alloc);
> diff --git a/net/ax25/ax25_in.c b/net/ax25/ax25_in.c
> index 5f1d210..de56d39 100644
> --- a/net/ax25/ax25_in.c
> +++ b/net/ax25/ax25_in.c
> @@ -437,8 +437,7 @@ free:
>  int ax25_kiss_rcv(struct sk_buff *skb, struct net_device *dev,
>  		  struct packet_type *ptype, struct net_device *orig_dev)
>  {
> -	skb->sk = NULL;		/* Initially we don't know who it's for */
> -	skb->destructor = NULL;	/* Who initializes this, dammit?! */
> +	skb_orphan(skb);
>  
>  	if (!net_eq(dev_net(dev), &init_net)) {
>  		kfree_skb(skb);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index baf2dc1..60b5728 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2310,8 +2310,6 @@ ncls:
>  	if (!skb)
>  		goto out;
>  
> -	skb_orphan(skb);
> -
>  	type = skb->protocol;
>  	list_for_each_entry_rcu(ptype,
>  			&ptype_base[ntohs(type) & PTYPE_HASH_MASK], list) {
> diff --git a/net/irda/af_irda.c b/net/irda/af_irda.c
> index 5922feb..cb762c8 100644
> --- a/net/irda/af_irda.c
> +++ b/net/irda/af_irda.c
> @@ -913,9 +913,6 @@ static int irda_accept(struct socket *sock, struct socket *newsock, int flags)
>  	/* Clean up the original one to keep it in listen state */
>  	irttp_listen(self->tsap);
>  
> -	/* Wow ! What is that ? Jean II */
> -	skb->sk = NULL;
> -	skb->destructor = NULL;
>  	kfree_skb(skb);
>  	sk->sk_ack_backlog--;
>  
> diff --git a/net/irda/ircomm/ircomm_lmp.c b/net/irda/ircomm/ircomm_lmp.c
> index 67c99d2..7ba9661 100644
> --- a/net/irda/ircomm/ircomm_lmp.c
> +++ b/net/irda/ircomm/ircomm_lmp.c
> @@ -196,6 +196,7 @@ static int ircomm_lmp_data_request(struct ircomm_cb *self,
>  	/* Don't forget to refcount it - see ircomm_tty_do_softint() */
>  	skb_get(skb);
>  
> +	skb_orphan(skb);
>  	skb->destructor = ircomm_lmp_flow_control;
>  
>  	if ((self->pkt_count++ > 7) && (self->flow_status == FLOW_START)) {
> 

Re: problem with 'net: Partially allow skb destructors to be used on receive path'

[Herbert Xu]

On Mon, Jun 22, 2009 at 03:44:38PM +0200, Oliver Hartkopp wrote:
>
> I tried to apply the patch on 2.6.30 also and only got some offsets (as expected).

Thanks for testing!

> Do you think, it's a good idea to queue this up for 2.6.30-stable?

If it's OK with Dave then it's OK with me.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: problem with 'net: Partially allow skb destructors to be used on receive path'

[David Miller]

From: Herbert Xu <herbert@gondor.apana.org.au>
Date: Mon, 22 Jun 2009 22:56:42 +0800

> On Mon, Jun 22, 2009 at 03:44:38PM +0200, Oliver Hartkopp wrote:
>>
>> I tried to apply the patch on 2.6.30 also and only got some offsets (as expected).
> 
> Thanks for testing!
> 
>> Do you think, it's a good idea to queue this up for 2.6.30-stable?
> 
> If it's OK with Dave then it's OK with me.

I've applied Herbert's patch, thanks everyone!

I'll queue it up for -stable too.

Re: problem with 'net: Partially allow skb destructors to be used on receive path'

[Oliver Hartkopp]

David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Date: Mon, 22 Jun 2009 22:56:42 +0800
> 
>> On Mon, Jun 22, 2009 at 03:44:38PM +0200, Oliver Hartkopp wrote:
>>> I tried to apply the patch on 2.6.30 also and only got some offsets (as expected).
>> Thanks for testing!
>>
>>> Do you think, it's a good idea to queue this up for 2.6.30-stable?
>> If it's OK with Dave then it's OK with me.
> 
> I've applied Herbert's patch, thanks everyone!
> 
> I'll queue it up for -stable too.

Hello Dave,

after a request from Matthias Fuchs regarding the status of this patch in
2.6.30-stable, i discovered a problem with the commit in 2.6.30-stable:

Patch discussion/history: http://patchwork.ozlabs.org/patch/28993/

The commit in mainline is correct:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=d55d87fdff8252d0e2f7c28c2d443aee17e9d70f

--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1250,6 +1250,7 @@ static inline int sk_has_allocations(const struct sock *sk)

 static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
 {
+       skb_orphan(skb);
        skb->sk = sk;
        skb->destructor = sock_wfree;
        /*
@@ -1262,6 +1263,7 @@ static inline void skb_set_owner_w(struct sk_buff *skb,
struct sock *sk)

 static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
+       skb_orphan(skb);
        skb->sk = sk;
        skb->destructor = sock_rfree;
        atomic_add(skb->truesize, &sk->sk_rmem_alloc);



But the commit in the 2.6.30-stable tree

http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.30.y.git;a=commitdiff;h=172570a224fe66d560c097e48fca15b620c76e72

has a problem in patching include/net/sock.h:


--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1231,6 +1231,8 @@ static inline void skb_set_owner_w(struct sk_buff *skb,
struct sock *sk)

 static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
+       skb_orphan(skb);
+       skb_orphan(skb);
        skb->sk = sk;
        skb->destructor = sock_rfree;
        atomic_add(skb->truesize, &sk->sk_rmem_alloc);


The skb_orphan(skb) in skb_set_owner_w() is missing here.

Is there any chance to fix that in 2.6.30-stable?

Thanks,
Oliver

Re: problem with 'net: Partially allow skb destructors to be used on receive path'

[David Miller]

From: Oliver Hartkopp <socketcan@hartkopp.net>
Date: Wed, 02 Dec 2009 22:21:44 +0100

> after a request from Matthias Fuchs regarding the status of this patch in
> 2.6.30-stable, i discovered a problem with the commit in 2.6.30-stable:
> 
> Patch discussion/history: http://patchwork.ozlabs.org/patch/28993/
> 
> The commit in mainline is correct:
 ...
> But the commit in the 2.6.30-stable tree
> 
> http://git.kernel.org/?p=linux/kernel/git/stable/linux-2.6.30.y.git;a=commitdiff;h=172570a224fe66d560c097e48fca15b620c76e72
> 
> has a problem in patching include/net/sock.h:
 ...
> The skb_orphan(skb) in skb_set_owner_w() is missing here.
> 
> Is there any chance to fix that in 2.6.30-stable?

Thanks for catching this.  I hate patch :-/

I'll send a fixup patch to the -stable folks right now.

Thanks again!

net: Fix thinko in backport of skb destructor fix.

As noticed by Oliver Hartkopp, the backport of the
'net: Partially allow skb destructors to be used on receive path'
(2.6.30.y commit: 172570a224fe66d560c097e48fca15b620c76e72,
 upstream commit: d55d87fdff8252d0e2f7c28c2d443aee17e9d70f)
was buggy.

It should have added an skb_orphan() call to both skb_set_owner_w()
and skb_set_owner_r().  Instead it added two calls to skb_set_owner_r().

This fixed it up.

Signed-off-by: David S. Miller <davem@davemloft.net>

diff --git a/include/net/sock.h b/include/net/sock.h
index 9bc2c83..cda3801 100644
--- a/include/net/sock.h
+++ b/include/net/sock.h
@@ -1223,6 +1223,7 @@ static inline int skb_copy_to_page(struct sock *sk, char __user *from,
 
 static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
 {
+	skb_orphan(skb);
 	sock_hold(sk);
 	skb->sk = sk;
 	skb->destructor = sock_wfree;
@@ -1232,7 +1233,6 @@ static inline void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
 static inline void skb_set_owner_r(struct sk_buff *skb, struct sock *sk)
 {
 	skb_orphan(skb);
-	skb_orphan(skb);
 	skb->sk = sk;
 	skb->destructor = sock_rfree;
 	atomic_add(skb->truesize, &sk->sk_rmem_alloc);