* [PATCH] appletalk: Set skb with destructor
@ 2014-07-05 4:28 Andrey Utkin
2014-07-05 20:28 ` Dan Carpenter
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Utkin @ 2014-07-05 4:28 UTC (permalink / raw)
To: linux-kernel, kernel-janitors, acme; +Cc: 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-05 4:28 [PATCH] appletalk: Set skb with destructor Andrey Utkin
@ 2014-07-05 20:28 ` Dan Carpenter
2014-07-06 11:56 ` Andrey Utkin
0 siblings, 1 reply; 10+ messages in thread
From: Dan Carpenter @ 2014-07-05 20:28 UTC (permalink / raw)
To: Andrey Utkin; +Cc: linux-kernel, kernel-janitors, acme
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] appletalk: Set skb with destructor
2014-07-05 20:28 ` Dan Carpenter
@ 2014-07-06 11:56 ` Andrey Utkin
2014-07-06 21:42 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Utkin @ 2014-07-06 11:56 UTC (permalink / raw)
To: linux-kernel, kernel-janitors, acme, netdev; +Cc: davem, 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-06 11:56 ` Andrey Utkin
@ 2014-07-06 21:42 ` Eric Dumazet
2014-07-07 8:03 ` Andrey Utkin
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2014-07-06 21:42 UTC (permalink / raw)
To: Andrey Utkin; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
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);
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-06 21:42 ` Eric Dumazet
@ 2014-07-07 8:03 ` Andrey Utkin
2014-07-07 8:57 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Utkin @ 2014-07-07 8:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-07 8:03 ` Andrey Utkin
@ 2014-07-07 8:57 ` Eric Dumazet
2014-07-07 9:26 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2014-07-07 8:57 UTC (permalink / raw)
To: Andrey Utkin; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
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 !
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-07 8:57 ` Eric Dumazet
@ 2014-07-07 9:26 ` Eric Dumazet
2014-07-07 10:02 ` Andrey Utkin
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2014-07-07 9:26 UTC (permalink / raw)
To: Andrey Utkin; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-07 9:26 ` Eric Dumazet
@ 2014-07-07 10:02 ` Andrey Utkin
2014-07-07 16:56 ` Eric Dumazet
0 siblings, 1 reply; 10+ messages in thread
From: Andrey Utkin @ 2014-07-07 10:02 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-07 10:02 ` Andrey Utkin
@ 2014-07-07 16:56 ` Eric Dumazet
2014-07-07 18:57 ` Andrey Utkin
0 siblings, 1 reply; 10+ messages in thread
From: Eric Dumazet @ 2014-07-07 16:56 UTC (permalink / raw)
To: Andrey Utkin; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
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;
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] appletalk: Set skb with destructor
2014-07-07 16:56 ` Eric Dumazet
@ 2014-07-07 18:57 ` Andrey Utkin
0 siblings, 0 replies; 10+ messages in thread
From: Andrey Utkin @ 2014-07-07 18:57 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linux-kernel, kernel-janitors, acme, netdev, davem
Thank you Eric. I have updated the bugzilla ticket with new patch,
will wait for test results from ticket author.
--
Andrey Utkin
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2014-07-07 18:58 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-07-05 4:28 [PATCH] appletalk: Set skb with destructor Andrey Utkin
2014-07-05 20:28 ` Dan Carpenter
2014-07-06 11:56 ` Andrey Utkin
2014-07-06 21:42 ` Eric Dumazet
2014-07-07 8:03 ` Andrey Utkin
2014-07-07 8:57 ` Eric Dumazet
2014-07-07 9:26 ` Eric Dumazet
2014-07-07 10:02 ` Andrey Utkin
2014-07-07 16:56 ` Eric Dumazet
2014-07-07 18:57 ` Andrey Utkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).