[PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[Dmitry Petukhov]

in function llc_sap_state_proces there was lack of return statement,
and finalizing kfree_skb might be called after skb was already freed
or queued to the user.

following patch adds the necessary return.

---

--- a/net/llc/llc_sap.c 2008-05-27 12:52:01.000000000 +0600
+++ b/net/llc/llc_sap.c 2008-05-27 12:52:37.000000000 +0600
@@ -223,6 +223,7 @@
                        if (sock_queue_rcv_skb(skb->sk, skb))
                                kfree_skb(skb);
                }
+               return;
        }
        kfree_skb(skb);
 }

Re: [PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[Wei Yongjun]

Dmitry Petukhov wrote:
> in function llc_sap_state_proces there was lack of return statement,
> and finalizing kfree_skb might be called after skb was already freed
> or queued to the user.
>
> following patch adds the necessary return.
>
>   

Not correct, since kfree_skb(skb) is used after skb_get(skb).

First, it used skb_get inc the users counter, and then, kfree_skb will 
dec the users count, not do the real free.

> ---
>
> --- a/net/llc/llc_sap.c 2008-05-27 12:52:01.000000000 +0600
> +++ b/net/llc/llc_sap.c 2008-05-27 12:52:37.000000000 +0600
> @@ -223,6 +223,7 @@
>                         if (sock_queue_rcv_skb(skb->sk, skb))
>                                 kfree_skb(skb);
>                 }
> +               return;
>         }
>         kfree_skb(skb);
>  }
>   

Re: [PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[Dmitry Petukhov]

2008/5/27 Wei Yongjun <yjwei@cn.fujitsu.com>:
> Not correct, since kfree_skb(skb) is used after skb_get(skb).
>
> First, it used skb_get inc the users counter, and then, kfree_skb will dec
> the users count, not do the real free.

The problem is what there will be one extra call to kfree_skb without
this return, and it will decrease the counter one extra time then.

//sorry for re-post, forgot to CC the list

Re: [PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[Dmitry Petukhov]

2008/5/27 Dmitry Petukhov <dmgenp@gmail.com>:
> 2008/5/27 Wei Yongjun <yjwei@cn.fujitsu.com>:
>
>> Normally,
>>
>> skb_get()         (**return )
>> kfree_skb()
>>
>> will do nothing. If you return with no kfree_skb(), it will let that skb can
>> not be free.
>>
>> skb_get()
>> kfree_skb()
>> kfree_skb()
>>
>> do the real free.
>
> Yeah, you're right. Looks like we mislocated the root of our problem
> (llc socket hangs on receive). Will debug further .
> Thanks for explanation.
>

Looks like we found the real root of our problem.
file net/llc/llc_sap.c:

skb_set_owner_r is called before llc_sap_rcv in two places (lines 363, 384)
skb_set_owner_r do this:
atomic_add(skb->truesize, &sk->sk_rmem_alloc);

and in llc_sap_state_process, on line 223 sock_queue_rcv_skb is
called, which also calls set_owner_r,
which in turn adds skb->truesize to sk->sk_rmem_alloc once more.
This double-addition results in sk_mem_alloc growth to exceed sk_rcvbuf.
We can observe this in  /proc/net/llc/socket, rx_queue field.
after this value exceeds sk_rcvbuf, sock_queue_rcv_skb always return
-ENOMEM, and
socket stops receiving.

//note: please CC me on reply, i'm not subscribed to the list.

Re: [PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[Arnaldo Carvalho de Melo]

Em Wed, May 28, 2008 at 04:59:45PM +0600, Dmitry Petukhov escreveu:
> 2008/5/27 Dmitry Petukhov <dmgenp@gmail.com>:
> > 2008/5/27 Wei Yongjun <yjwei@cn.fujitsu.com>:
> >
> >> Normally,
> >>
> >> skb_get()         (**return )
> >> kfree_skb()
> >>
> >> will do nothing. If you return with no kfree_skb(), it will let that skb can
> >> not be free.
> >>
> >> skb_get()
> >> kfree_skb()
> >> kfree_skb()
> >>
> >> do the real free.
> >
> > Yeah, you're right. Looks like we mislocated the root of our problem
> > (llc socket hangs on receive). Will debug further .
> > Thanks for explanation.
> >
> 
> Looks like we found the real root of our problem.
> file net/llc/llc_sap.c:
> 
> skb_set_owner_r is called before llc_sap_rcv in two places (lines 363, 384)
> skb_set_owner_r do this:
> atomic_add(skb->truesize, &sk->sk_rmem_alloc);
> 
> and in llc_sap_state_process, on line 223 sock_queue_rcv_skb is
> called, which also calls set_owner_r,
> which in turn adds skb->truesize to sk->sk_rmem_alloc once more.
> This double-addition results in sk_mem_alloc growth to exceed sk_rcvbuf.
> We can observe this in  /proc/net/llc/socket, rx_queue field.
> after this value exceeds sk_rcvbuf, sock_queue_rcv_skb always return
> -ENOMEM, and
> socket stops receiving.
> 
> //note: please CC me on reply, i'm not subscribed to the list.

Good catch, that code needs some revisiting after all these years...
Anyway, can you please try this patch and report results? Compile tested
only:

diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
index e2ddde7..008de1f 100644
--- a/net/llc/llc_sap.c
+++ b/net/llc/llc_sap.c
@@ -286,12 +286,14 @@ void llc_build_and_send_xid_pkt(struct llc_sap *sap, struct sk_buff *skb,
  *
  *	Sends received pdus to the sap state machine.
  */
-static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb)
+static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb,
+			struct sock *sk)
 {
 	struct llc_sap_state_ev *ev = llc_sap_ev(skb);
 
 	ev->type   = LLC_SAP_EV_TYPE_PDU;
 	ev->reason = 0;
+	skb->sk = sk;
 	llc_sap_state_process(sap, skb);
 }
 
@@ -360,8 +362,7 @@ static void llc_sap_mcast(struct llc_sap *sap,
 			break;
 
 		sock_hold(sk);
-		skb_set_owner_r(skb1, sk);
-		llc_sap_rcv(sap, skb1);
+		llc_sap_rcv(sap, skb1, sk);
 		sock_put(sk);
 	}
 	read_unlock_bh(&sap->sk_list.lock);
@@ -381,8 +382,7 @@ void llc_sap_handler(struct llc_sap *sap, struct sk_buff *skb)
 	} else {
 		struct sock *sk = llc_lookup_dgram(sap, &laddr);
 		if (sk) {
-			skb_set_owner_r(skb, sk);
-			llc_sap_rcv(sap, skb);
+			llc_sap_rcv(sap, skb, sk);
 			sock_put(sk);
 		} else
 			kfree_skb(skb);

Re: [PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[Dmitry Petukhov]

2008/5/29 Arnaldo Carvalho de Melo <acme@redhat.com>:

> Good catch, that code needs some revisiting after all these years...
> Anyway, can you please try this patch and report results?

problem disappear after patch applied.

Re: [PATCH 2.6.26-rc4] fix double call of kfree_skb in net/llc/llc_sap.c

[David Miller]

From: "Dmitry Petukhov" <dmgenp@gmail.com>
Date: Thu, 29 May 2008 14:51:59 +0600

> 2008/5/29 Arnaldo Carvalho de Melo <acme@redhat.com>:
> 
> > Good catch, that code needs some revisiting after all these years...
> > Anyway, can you please try this patch and report results?
> 
> problem disappear after patch applied.

Arnaldo, please send the final version of this patch with
proper changelogs and signoffs, thanks!

[PATCH][LLC]: Fix double accounting of received packets

[Arnaldo Carvalho de Melo]

David, please apply.

- Arnaldo

commit 3eb2a480af55fe7494c1601b6b7eda499cd67ddd
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Thu May 29 10:33:00 2008 -0300

    [LLC]: Fix double accounting of received packets
    
    llc_sap_rcv was being preceded by skb_set_owner_r, then calling
    llc_state_process that calls sock_queue_rcv_skb, that in turn calls
    skb_set_owner_r again making the space allowed to be used by the socket to be
    leaked, making the socket to get stuck.
    
    Fix it by setting skb->sk at llc_sap_rcv and leave the accounting to be done
    only at sock_queue_rcv_skb.
    
    Reported-by: Dmitry Petukhov <dmgenp@gmail.com>
    Tested-by: Dmitry Petukhov <dmgenp@gmail.com>
    Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/net/llc/llc_sap.c b/net/llc/llc_sap.c
index e2ddde7..008de1f 100644
--- a/net/llc/llc_sap.c
+++ b/net/llc/llc_sap.c
@@ -286,12 +286,14 @@ void llc_build_and_send_xid_pkt(struct llc_sap *sap, struct sk_buff *skb,
  *
  *	Sends received pdus to the sap state machine.
  */
-static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb)
+static void llc_sap_rcv(struct llc_sap *sap, struct sk_buff *skb,
+			struct sock *sk)
 {
 	struct llc_sap_state_ev *ev = llc_sap_ev(skb);
 
 	ev->type   = LLC_SAP_EV_TYPE_PDU;
 	ev->reason = 0;
+	skb->sk = sk;
 	llc_sap_state_process(sap, skb);
 }
 
@@ -360,8 +362,7 @@ static void llc_sap_mcast(struct llc_sap *sap,
 			break;
 
 		sock_hold(sk);
-		skb_set_owner_r(skb1, sk);
-		llc_sap_rcv(sap, skb1);
+		llc_sap_rcv(sap, skb1, sk);
 		sock_put(sk);
 	}
 	read_unlock_bh(&sap->sk_list.lock);
@@ -381,8 +382,7 @@ void llc_sap_handler(struct llc_sap *sap, struct sk_buff *skb)
 	} else {
 		struct sock *sk = llc_lookup_dgram(sap, &laddr);
 		if (sk) {
-			skb_set_owner_r(skb, sk);
-			llc_sap_rcv(sap, skb);
+			llc_sap_rcv(sap, skb, sk);
 			sock_put(sk);
 		} else
 			kfree_skb(skb);

Re: [PATCH][LLC]: Fix double accounting of received packets

[David Miller]

From: Arnaldo Carvalho de Melo <acme@redhat.com>
Date: Thu, 29 May 2008 10:44:41 -0300

> David, please apply.

Applied, thanks a lot!