[PATCH] appletalk: Set skb with destructor

[PATCH] appletalk: Set skb with destructor

[Andrey Utkin]

See https://bugzilla.kernel.org/show_bug.cgi?id=79441
---8<---
Made changes similar to 0ae89beb283a0db5980d1d4781c7d7be2f2810d6

Reported-by: Ed Martin <edman007@edman007.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
---
 net/appletalk/ddp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 01a1082..3d8ab34 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1399,6 +1399,11 @@ drop:
 	return NET_RX_DROP;
 }
 
+static inline void atalk_skb_destructor(struct sk_buff *skb)
+{
+	sock_put(skb->sk);
+}
+
 /**
  *	atalk_rcv - Receive a packet (in skb) from device dev
  *	@skb - packet received
@@ -1489,6 +1494,8 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
 		goto drop;
 
 	/* Queue packet (standard) */
+	sock_hold(sock);
+	skb->destructor = atalk_skb_destructor;
 	skb->sk = sock;
 
 	if (sock_queue_rcv_skb(sock, skb) < 0)
@@ -1644,6 +1651,8 @@ static int atalk_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
 	if (!skb)
 		goto out;
 
+	sock_hold(sk);
+	skb->destructor = atalk_skb_destructor;
 	skb->sk = sk;
 	skb_reserve(skb, ddp_dl->header_length);
 	skb_reserve(skb, dev->hard_header_len);
-- 
1.8.3.2

Re: [PATCH] appletalk: Set skb with destructor

[Dan Carpenter]

On Sat, Jul 05, 2014 at 07:28:14AM +0300, Andrey Utkin wrote:
> See https://bugzilla.kernel.org/show_bug.cgi?id=79441
> ---8<---
> Made changes similar to 0ae89beb283a0db5980d1d4781c7d7be2f2810d6
> 

Thanks for fixing this bug but the patch description is just a URL and a
git hash.  Say something like:

The sock ref counting is off so there is a kernel panic when you run
`atalkd`.  See https://bugzilla.kernel.org/show_bug.cgi?id=79441
This fix is similar to 0ae89beb283a ('can: add destructor for self
generated skbs')

--------------------------

Putting a little information directly in the changelog that it is about
refcounting and panics is a useful thing.

Also you need to send this to netdev@vger.kernel.org and CC
"David S. Miller" <davem@davemloft.net>.  Otherwise the patch looks good
to my non-expert eye.  Please resend to with the updated changelog and
CC list.

regards,
dan carpenter

[PATCH] appletalk: Set skb with destructor

[Andrey Utkin]

The sock ref counting is off so there is a kernel panic when you run
`atalkd`.  See https://bugzilla.kernel.org/show_bug.cgi?id=79441
This fix is similar to 0ae89beb283a ('can: add destructor for self
generated skbs')

Reported-by: Ed Martin <edman007@edman007.com>
Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
---
 net/appletalk/ddp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 01a1082..3d8ab34 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1399,6 +1399,11 @@ drop:
 	return NET_RX_DROP;
 }
 
+static inline void atalk_skb_destructor(struct sk_buff *skb)
+{
+	sock_put(skb->sk);
+}
+
 /**
  *	atalk_rcv - Receive a packet (in skb) from device dev
  *	@skb - packet received
@@ -1489,6 +1494,8 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
 		goto drop;
 
 	/* Queue packet (standard) */
+	sock_hold(sock);
+	skb->destructor = atalk_skb_destructor;
 	skb->sk = sock;
 
 	if (sock_queue_rcv_skb(sock, skb) < 0)
@@ -1644,6 +1651,8 @@ static int atalk_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
 	if (!skb)
 		goto out;
 
+	sock_hold(sk);
+	skb->destructor = atalk_skb_destructor;
 	skb->sk = sk;
 	skb_reserve(skb, ddp_dl->header_length);
 	skb_reserve(skb, dev->hard_header_len);
-- 
1.8.3.2

Re: [PATCH] appletalk: Set skb with destructor

[Eric Dumazet]

On Sun, 2014-07-06 at 14:56 +0300, Andrey Utkin wrote:
> The sock ref counting is off so there is a kernel panic when you run
> `atalkd`.  See https://bugzilla.kernel.org/show_bug.cgi?id=79441
> This fix is similar to 0ae89beb283a ('can: add destructor for self
> generated skbs')
>  should
> Reported-by: Ed Martin <edman007@edman007.com>
> Signed-off-by: Andrey Utkin <andrey.krieger.utkin@gmail.com>
> ---
>  net/appletalk/ddp.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
> index 01a1082..3d8ab34 100644
> --- a/net/appletalk/ddp.c
> +++ b/net/appletalk/ddp.c
> @@ -1399,6 +1399,11 @@ drop:
>  	return NET_RX_DROP;
>  }
>  
> +static inline void atalk_skb_destructor(struct sk_buff *skb)
> +{
> +	sock_put(skb->sk);
> +}
> +
>  /**
>   *	atalk_rcv - Receive a packet (in skb) from device dev
>   *	@skb - packet received
> @@ -1489,6 +1494,8 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
>  		goto drop;
>  
>  	/* Queue packet (standard) */
> +	sock_hold(sock);
> +	skb->destructor = atalk_skb_destructor;
>  	skb->sk = sock;

This part is not needed : sock_queue_rcv_skb() already does the right
thing : It calls skb_set_owner_r(skb, sk);

You should therefore remove the "skb->sk = sock;" line

>  
>  	if (sock_queue_rcv_skb(sock, skb) < 0)
> @@ -1644,6 +1651,8 @@ static int atalk_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
>  	if (!skb)
>  		goto out;
>  
> +	sock_hold(sk);
> +	skb->destructor = atalk_skb_destructor;
>  	skb->sk = sk;
>  	skb_reserve(skb, ddp_dl->header_length);
>  	skb_reserve(skb, dev->hard_header_len);

Re: [PATCH] appletalk: Set skb with destructor

[Andrey Utkin]

2014-07-07 0:42 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>:
>>       /* Queue packet (standard) */
>> +     sock_hold(sock);
>> +     skb->destructor = atalk_skb_destructor;
>>       skb->sk = sock;
>
> This part is not needed : sock_queue_rcv_skb() already does the right
> thing : It calls skb_set_owner_r(skb, sk);
>
> You should therefore remove the "skb->sk = sock;" line

If it is so, i think this should be as another patch with its own reasoning.

-- 
Andrey Utkin

Re: [PATCH] appletalk: Set skb with destructor

[Eric Dumazet]

On Mon, 2014-07-07 at 11:03 +0300, Andrey Utkin wrote:
> 2014-07-07 0:42 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>:
> >>       /* Queue packet (standard) */
> >> +     sock_hold(sock);
> >> +     skb->destructor = atalk_skb_destructor;
> >>       skb->sk = sock;
> >
> > This part is not needed : sock_queue_rcv_skb() already does the right
> > thing : It calls skb_set_owner_r(skb, sk);
> >
> > You should therefore remove the "skb->sk = sock;" line
> 
> If it is so, i think this should be as another patch with its own reasoning.
> 

No it is not.

Its illegal to set skb->sk to a socket without taking proper reference.

But it is useless to do this before calling sock_queue_rcv_skb(), as I
explained to you.

This is adding two extra atomic operations for nothing: skb_orphan() is
called from sock_queue_rcv_skb(), so it is kind of stupid to set a
destructor that _will_ be immediately called.

We do not do defensive programming, we try to do logical things, and
only logical things.

Please re-spin your patch, by integrating my feedback.

Thanks !

Re: [PATCH] appletalk: Set skb with destructor

[Eric Dumazet]

On Mon, 2014-07-07 at 10:57 +0200, Eric Dumazet wrote:
> On Mon, 2014-07-07 at 11:03 +0300, Andrey Utkin wrote:
> > 2014-07-07 0:42 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>:
> > >>       /* Queue packet (standard) */
> > >> +     sock_hold(sock);
> > >> +     skb->destructor = atalk_skb_destructor;
> > >>       skb->sk = sock;
> > >
> > > This part is not needed : sock_queue_rcv_skb() already does the right
> > > thing : It calls skb_set_owner_r(skb, sk);
> > >
> > > You should therefore remove the "skb->sk = sock;" line
> > 
> > If it is so, i think this should be as another patch with its own reasoning.
> > 
> 
> No it is not.
> 
> Its illegal to set skb->sk to a socket without taking proper reference.
> 
> But it is useless to do this before calling sock_queue_rcv_skb(), as I
> explained to you.
> 
> This is adding two extra atomic operations for nothing: skb_orphan() is
> called from sock_queue_rcv_skb(), so it is kind of stupid to set a
> destructor that _will_ be immediately called.
> 
> We do not do defensive programming, we try to do logical things, and
> only logical things.
> 
> Please re-spin your patch, by integrating my feedback.
> 
> Thanks !

Reading again this code, I think all you need is to remove the 2 buggy
lines.

No need for setup destructors.

Re: [PATCH] appletalk: Set skb with destructor

[Andrey Utkin]

2014-07-07 12:26 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>:
> Reading again this code, I think all you need is to remove the 2 buggy
> lines.
>
> No need for setup destructors.

Reviewing the code again, i find you're right and adding a destructor
in that place is stupid, and just not setting skb->sk would make bug
disappear, assuming skb->sk is previously NULL.
And what about the second case, at ddp.c near line 1650? Is destructor
needed here?

-- 
Andrey Utkin

Re: [PATCH] appletalk: Set skb with destructor

[Eric Dumazet]

On Mon, 2014-07-07 at 13:02 +0300, Andrey Utkin wrote:
> 2014-07-07 12:26 GMT+03:00 Eric Dumazet <eric.dumazet@gmail.com>:
> > Reading again this code, I think all you need is to remove the 2 buggy
> > lines.
> >
> > No need for setup destructors.
> 
> Reviewing the code again, i find you're right and adding a destructor
> in that place is stupid, and just not setting skb->sk would make bug
> disappear, assuming skb->sk is previously NULL.
> And what about the second case, at ddp.c near line 1650? Is destructor
> needed here?

Not needed at all.

There is already a proper destructor set by sock_alloc_send_skb()

As I said, all you need is to remove the 2 lines.

Something like :

diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 01a1082e02b3..5563dcb7a1fc 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1489,7 +1489,6 @@ static int atalk_rcv(struct sk_buff *skb, struct net_device *dev,
 		goto drop;
 
 	/* Queue packet (standard) */
-	skb->sk = sock;
 
 	if (sock_queue_rcv_skb(sock, skb) < 0)
 		goto drop;
@@ -1644,7 +1643,6 @@ static int atalk_sendmsg(struct kiocb *iocb, struct socket *sock, struct msghdr
 	if (!skb)
 		goto out;
 
-	skb->sk = sk;
 	skb_reserve(skb, ddp_dl->header_length);
 	skb_reserve(skb, dev->hard_header_len);
 	skb->dev = dev;

Re: [PATCH] appletalk: Set skb with destructor

[Andrey Utkin]

Thank you Eric. I have updated the bugzilla ticket with new patch,
will wait for test results from ticket author.

-- 
Andrey Utkin