fix double call of kfree_skb in net/llc/llc_sap.c
Subsystems (3)
PATCH 1/1 fix double call of kfree_skb in net/llc/llc_sap.c
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.
---
net/llc/llc_sap.c 2008-05-27 12:52:37.000000000 +0600 renamed from net/llc/llc_sap.c 2008-05-27 12:52:01.000000000 +0600 +1 -0
@@ -223,6 +223,7 @@ if (sock_queue_rcv_skb(skb->sk, skb)) kfree_skb(skb); } + return; } kfree_skb(skb); }
Quoted 7 lines · Dmitry Petukhov
in function
llc_sap_state_procesthere was lack of return statement, and finalizingkfree_skbmight 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.
Quoted 13 lines
---
net/llc/llc_sap.c 2008-05-27 12:52:37.000000000 +0600 renamed from net/llc/llc_sap.c 2008-05-27 12:52:01.000000000 +0600 +1
-0@@ -223,6 +223,7 @@ if (sock_queue_rcv_skb(skb->sk, skb)) kfree_skb(skb); } + return; } kfree_skb(skb); }
2008/5/27 Wei Yongjun <[email protected]>:
Quoted 4 lines
Not correct, since
kfree_skb(skb)is used afterskb_get(skb).First, it used
skb_getinc the users counter, and then,kfree_skbwill 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
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): [email protected]
2008/5/27 Dmitry Petukhov <[email protected]>:
Quoted 14 lines
2008/5/27 Wei Yongjun <[email protected]>:
Quoted 12 lines
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()
Quoted 6 lines
Quoted 1 line
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.
Em Wed, May 28, 2008 at 04:59:45PM +0600, Dmitry Petukhov escreveu:
Quoted 38 lines
2008/5/27 Dmitry Petukhov <[email protected]>:
Quoted 14 lines
2008/5/27 Wei Yongjun <[email protected]>:
Quoted 12 lines
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()Quoted 6 lines
Quoted 1 line
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.
filenet/llc/llc_sap.c:
skb_set_owner_ris called beforellc_sap_rcvin two places (lines 363, 384)skb_set_owner_rdo this:atomic_add(skb->truesize, &sk->sk_rmem_alloc);and in
llc_sap_state_process, on line 223sock_queue_rcv_skbis called, which also callsset_owner_r,
which in turn addsskb->truesizetosk->sk_rmem_alloconce more. This double-addition results insk_mem_allocgrowth to exceedsk_rcvbuf. We can observe this in/proc/net/llc/socket,rx_queuefield. after this value exceedssk_rcvbuf,sock_queue_rcv_skbalways return-ENOMEM, and
socket stops receiving.
Quoted 1 line
//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:
net/llc/llc_sap.c +5 -5
@@ -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);
2008/5/29 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 2 lines
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.
David Miller
maintainerFrom: "Dmitry Petukhov" <[email protected]>
Date: Thu, 29 May 2008 14:51:59 +0600
Quoted 6 lines
2008/5/29 Arnaldo Carvalho de Melo <[email protected]>:
Quoted 2 lines
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 1/1 : Fix double accounting of received packets
David, please apply.
- Arnaldo
commit 3eb2a480af55fe7494c1601b6b7eda499cd67ddd
Author: Arnaldo Carvalho de Melo <[email protected]> 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.
net/llc/llc_sap.c +5 -5
@@ -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
From: Arnaldo Carvalho de Melo <[email protected]>
Date: Thu, 29 May 2008 10:44:41 -0300
Quoted 1 line
David, please apply.
Applied, thanks a lot!