See https://bugzilla.kernel.org/show_bug.cgi?id=79441
---8<---
Made changes similar to 0ae89beb283a0db5980d1d4781c7d7be2f2810d6
Reported-by: Ed Martin <[email protected]>
Signed-off-by: Andrey Utkin <[email protected]>
---
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
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 [email protected] and CC
"David S. Miller" <[email protected]>. Otherwise the patch looks good
to my non-expert eye. Please resend to with the updated changelog and
CC list.
regards,
dan carpenter
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 <[email protected]>
Signed-off-by: Andrey Utkin <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Andrey Utkin <[email protected]>
> ---
> 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);
2014-07-07 0:42 GMT+03:00 Eric Dumazet <[email protected]>:
>> /* 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
On Mon, 2014-07-07 at 11:03 +0300, Andrey Utkin wrote:
> 2014-07-07 0:42 GMT+03:00 Eric Dumazet <[email protected]>:
> >> /* 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 !
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 <[email protected]>:
> > >> /* 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.
2014-07-07 12:26 GMT+03:00 Eric Dumazet <[email protected]>:
> 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
On Mon, 2014-07-07 at 13:02 +0300, Andrey Utkin wrote:
> 2014-07-07 12:26 GMT+03:00 Eric Dumazet <[email protected]>:
> > 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;
Thank you Eric. I have updated the bugzilla ticket with new patch,
will wait for test results from ticket author.
--
Andrey Utkin