RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi Wang cong, After apply the patch, I did not hit the issue again. Thank you~ Best Regards, liujian > -Original Message- > From: Dingtianhong > Sent: Monday, July 24, 2017 9:29 AM > To: Cong Wang; liujian (CE) > Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com; > da...@davemloft.net; eduma...@google.com; will...@google.com; > dan...@iogearbox.net; netdev@vger.kernel.org; linux-ker...@vger.kernel.org > Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired > > > > On 2017/7/24 9:09, Ding Tianhong wrote: > > > > > > On 2017/7/24 1:03, Cong Wang wrote: > >> On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) wrote: > >>> Hi > >>> > >>> I find it caused by below steps: > >>> 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 2. set > >>> tp_block_nr to 0 Then pg_vec was freed, and we did not delete the > >>> timer? > >> > >> Thanks for testing! > >> > >> Ah, I overlook the initialization case in my previous patch. > >> > >> How about the following one? Does it cover all the cases? > >> > >> > >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index > >> 008bb34ee324..0615c2a950fa 100644 > >> --- a/net/packet/af_packet.c > >> +++ b/net/packet/af_packet.c > >> @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk, > >> union tpacket_req_u *req_u, > >> register_prot_hook(sk); > >> } > >> spin_unlock(&po->bind_lock); > >> - if (closing && (po->tp_version > TPACKET_V2)) { > >> + if (pg_vec && (po->tp_version > TPACKET_V2)) { > >> /* Because we don't support block-based V3 on tx-ring > */ > >> if (!tx_ring) > >> prb_shutdown_retire_blk_timer(po, > rb_queue); > >> > >> . > > > > Hi, Cong: > > > > It looks like could not cover the case: req->tp_block_nr = 2 -> > reg->tp_block_nr = 1 . > > > > Oh, looks like this case would never happen, so I think your solution is ok. > > > what about this way: > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -4331,13 +4331,17 @@ static int packet_set_ring(struct sock *sk, union > tpacket_req_u *req_u, > > register_prot_hook(sk); > > } > > spin_unlock(&po->bind_lock); > > - if (closing && (po->tp_version > TPACKET_V2)) { > > + if ((closing || (pg_vec && !reg->tp_block_nr))&& > > + (po->tp_version > TPACKET_V2)) { > > /* Because we don't support block-based V3 on tx-ring */ > > if (!tx_ring) > > prb_shutdown_retire_blk_timer(po, > rb_queue); > > > > > > >> > > > > > > . > >
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On 2017/7/24 9:09, Ding Tianhong wrote: > > > On 2017/7/24 1:03, Cong Wang wrote: >> On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) wrote: >>> Hi >>> >>> I find it caused by below steps: >>> 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 >>> 2. set tp_block_nr to 0 >>> Then pg_vec was freed, and we did not delete the timer? >> >> Thanks for testing! >> >> Ah, I overlook the initialization case in my previous patch. >> >> How about the following one? Does it cover all the cases? >> >> >> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c >> index 008bb34ee324..0615c2a950fa 100644 >> --- a/net/packet/af_packet.c >> +++ b/net/packet/af_packet.c >> @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk, >> union tpacket_req_u *req_u, >> register_prot_hook(sk); >> } >> spin_unlock(&po->bind_lock); >> - if (closing && (po->tp_version > TPACKET_V2)) { >> + if (pg_vec && (po->tp_version > TPACKET_V2)) { >> /* Because we don't support block-based V3 on tx-ring */ >> if (!tx_ring) >> prb_shutdown_retire_blk_timer(po, rb_queue); >> >> . > > Hi, Cong: > > It looks like could not cover the case: req->tp_block_nr = 2 -> > reg->tp_block_nr = 1 . > Oh, looks like this case would never happen, so I think your solution is ok. > what about this way: > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -4331,13 +4331,17 @@ static int packet_set_ring(struct sock *sk, union > tpacket_req_u *req_u, > register_prot_hook(sk); > } > spin_unlock(&po->bind_lock); > - if (closing && (po->tp_version > TPACKET_V2)) { > + if ((closing || (pg_vec && !reg->tp_block_nr))&& (po->tp_version > > TPACKET_V2)) { > /* Because we don't support block-based V3 on tx-ring */ > if (!tx_ring) > prb_shutdown_retire_blk_timer(po, rb_queue); > > >> > > > . >
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On 2017/7/24 1:03, Cong Wang wrote: > On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) wrote: >> Hi >> >> I find it caused by below steps: >> 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 >> 2. set tp_block_nr to 0 >> Then pg_vec was freed, and we did not delete the timer? > > Thanks for testing! > > Ah, I overlook the initialization case in my previous patch. > > How about the following one? Does it cover all the cases? > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 008bb34ee324..0615c2a950fa 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk, > union tpacket_req_u *req_u, > register_prot_hook(sk); > } > spin_unlock(&po->bind_lock); > - if (closing && (po->tp_version > TPACKET_V2)) { > + if (pg_vec && (po->tp_version > TPACKET_V2)) { > /* Because we don't support block-based V3 on tx-ring */ > if (!tx_ring) > prb_shutdown_retire_blk_timer(po, rb_queue); > > . Hi, Cong: It looks like could not cover the case: req->tp_block_nr = 2 -> reg->tp_block_nr = 1 . what about this way: --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4331,13 +4331,17 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u, register_prot_hook(sk); } spin_unlock(&po->bind_lock); - if (closing && (po->tp_version > TPACKET_V2)) { + if ((closing || (pg_vec && !reg->tp_block_nr))&& (po->tp_version > TPACKET_V2)) { /* Because we don't support block-based V3 on tx-ring */ if (!tx_ring) prb_shutdown_retire_blk_timer(po, rb_queue); >
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On Sun, Jul 23, 2017 at 5:48 AM, liujian (CE) wrote: > Hi > > I find it caused by below steps: > 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 > 2. set tp_block_nr to 0 > Then pg_vec was freed, and we did not delete the timer? Thanks for testing! Ah, I overlook the initialization case in my previous patch. How about the following one? Does it cover all the cases? diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 008bb34ee324..0615c2a950fa 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4329,7 +4329,7 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u, register_prot_hook(sk); } spin_unlock(&po->bind_lock); - if (closing && (po->tp_version > TPACKET_V2)) { + if (pg_vec && (po->tp_version > TPACKET_V2)) { /* Because we don't support block-based V3 on tx-ring */ if (!tx_ring) prb_shutdown_retire_blk_timer(po, rb_queue);
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi I find it caused by below steps: 1. set tp_version to TPACKET_V3 and req->tp_block_nr to 1 2. set tp_block_nr to 0 Then pg_vec was freed, and we did not delete the timer? Best Regards, liujian > -Original Message- > From: liujian (CE) > Sent: Sunday, July 23, 2017 5:47 PM > To: liujian (CE); 'Cong Wang'; Dingtianhong > Cc: 'Willem de Bruijn'; 'Dave Jones'; 'alexander.le...@verizon.com'; > 'da...@davemloft.net'; 'eduma...@google.com'; 'will...@google.com'; > 'dan...@iogearbox.net'; 'netdev@vger.kernel.org'; > 'linux-ker...@vger.kernel.org' > Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired > > Hi, > > Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to > TPACKET_V1 ? > > > Best Regards, > liujian > > > > -Original Message- > > From: liujian (CE) > > Sent: Sunday, July 23, 2017 4:21 PM > > To: 'Cong Wang'; Dingtianhong > > Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com; > > da...@davemloft.net; eduma...@google.com; will...@google.com; > > dan...@iogearbox.net; netdev@vger.kernel.org; > > linux-ker...@vger.kernel.org > > Subject: RE: af_packet: use after free in > > prb_retire_rx_blk_timer_expired > > > > Hi Wang Cong, > > > > With this patch , the system was crashed when setsockopt. > > > > The call trace as below: > > > > crash> bt > > PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main" > > #0 [8801bec03ce0] machine_kexec at 8105354b > > #1 [8801bec03d40] crash_kexec at 810f7e82 > > #2 [8801bec03e10] panic at 81650058 > > #3 [8801bec03e90] watchdog_timer_fn at 81122533 > > #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2 > > #5 [8801bec03f20] hrtimer_interrupt at 810ac450 > > #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457 > > #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0 > > #8 [8801bec03fb0] apic_timer_interrupt at 8166931d > > --- --- > > #9 [8801b301fcb8] apic_timer_interrupt at 8166931d > > [exception RIP: lock_timer_base+77] > > RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246 > > RAX: RBX: 8800afcc RCX: > > 0001 > > RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8 > > RBP: 8801b301fd80 R8: 8800afcc R9: > ea000268 > > R10: 003c R11: 8801b301fb2e R12: > 8800afcc > > R13: 8800afcc R14: R15: > 83d1a340 > > ORIG_RAX: ff10 CS: 0010 SS: 0018 > > #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f > > #11 [8801b301fdb8] del_timer_sync at 8108f252 > > #12 [8801b301fdd0] packet_set_ring at 81635e60 > > #13 [8801b301fe98] packet_setsockopt at 81636760 > > #14 [8801b301ff38] sys_setsockopt at 81531860 > > #15 [8801b301ff80] tracesys at 816687ed (via system_call) > > RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202 > > RAX: ffda RBX: 816687ed RCX: > > RDX: 0005 RSI: 0107 RDI: > > 0180 > > RBP: 0180 R8: 001c R9: > > 7fcc78dc7160 > > R10: 01fd6ba0 R11: 0202 R12: > > > > R13: 0011 R14: 01fd6b60 R15: > > 01fd6b70 > > ORIG_RAX: 0036 CS: 0033 SS: 002b > > > > > > Best Regards, > > liujian > > > > > > > -Original Message- > > > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > > > Sent: Sunday, July 23, 2017 1:59 PM > > > To: Dingtianhong > > > Cc: liujian (CE); Willem de Bruijn; Dave Jones; > > > alexander.le...@verizon.com; da...@davemloft.net; > > eduma...@google.com; > > > will...@google.com; dan...@iogearbox.net; netdev@vger.kernel.org; > > > linux-ker...@vger.kernel.org > > > Subject: Re: af_packet: use after free in > > > prb_retire_rx_blk_timer_expired > > > > > > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong > > > > > > wrote: > > > > Hi, Cong: > > > > > > > > Thanks for your quirk solution, but I still has some doubts
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi, Do we need delete the v3 ring, when tp_version changed from TPACKET_V3 to TPACKET_V1 ? Best Regards, liujian > -Original Message- > From: liujian (CE) > Sent: Sunday, July 23, 2017 4:21 PM > To: 'Cong Wang'; Dingtianhong > Cc: Willem de Bruijn; Dave Jones; alexander.le...@verizon.com; > da...@davemloft.net; eduma...@google.com; will...@google.com; > dan...@iogearbox.net; netdev@vger.kernel.org; linux-ker...@vger.kernel.org > Subject: RE: af_packet: use after free in prb_retire_rx_blk_timer_expired > > Hi Wang Cong, > > With this patch , the system was crashed when setsockopt. > > The call trace as below: > > crash> bt > PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main" > #0 [8801bec03ce0] machine_kexec at 8105354b > #1 [8801bec03d40] crash_kexec at 810f7e82 > #2 [8801bec03e10] panic at 81650058 > #3 [8801bec03e90] watchdog_timer_fn at 81122533 > #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2 > #5 [8801bec03f20] hrtimer_interrupt at 810ac450 > #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457 > #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0 > #8 [8801bec03fb0] apic_timer_interrupt at 8166931d > --- --- > #9 [8801b301fcb8] apic_timer_interrupt at 8166931d > [exception RIP: lock_timer_base+77] > RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246 > RAX: RBX: 8800afcc RCX: > 0001 > RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8 > RBP: 8801b301fd80 R8: 8800afcc R9: ea000268 > R10: 003c R11: 8801b301fb2e R12: 8800afcc > R13: 8800afcc R14: R15: 83d1a340 > ORIG_RAX: ff10 CS: 0010 SS: 0018 > #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f > #11 [8801b301fdb8] del_timer_sync at 8108f252 > #12 [8801b301fdd0] packet_set_ring at 81635e60 > #13 [8801b301fe98] packet_setsockopt at 81636760 > #14 [8801b301ff38] sys_setsockopt at 81531860 > #15 [8801b301ff80] tracesys at 816687ed (via system_call) > RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202 > RAX: ffda RBX: 816687ed RCX: > RDX: 0005 RSI: 0107 RDI: > 0180 > RBP: 0180 R8: 001c R9: > 7fcc78dc7160 > R10: 01fd6ba0 R11: 0202 R12: > > R13: 0011 R14: 01fd6b60 R15: > 01fd6b70 > ORIG_RAX: 0036 CS: 0033 SS: 002b > > > Best Regards, > liujian > > > > -Original Message- > > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > > Sent: Sunday, July 23, 2017 1:59 PM > > To: Dingtianhong > > Cc: liujian (CE); Willem de Bruijn; Dave Jones; > > alexander.le...@verizon.com; da...@davemloft.net; > eduma...@google.com; > > will...@google.com; dan...@iogearbox.net; netdev@vger.kernel.org; > > linux-ker...@vger.kernel.org > > Subject: Re: af_packet: use after free in > > prb_retire_rx_blk_timer_expired > > > > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong > > > > wrote: > > > Hi, Cong: > > > > > > Thanks for your quirk solution, but I still has some doubts about > > > it, it looks like fix the problem in the > > > packet_setsockopt->packet_set_ring processing, but when in > > > packet_release processing, it may could not release the real pg_vec > > > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss > > > something here, nice to hear from your feedback. :) > > > > Yes you miss that packet_release() has memset()'s so we won't hit that > > path. :) > > > > However, I missed the swap() in this messy function, actually I > > believe the bug is that we modify tpacket_kbdq_core inside rx_ring in > > non-closing case without actually stopping its timer. I feel more confident > with the following patch: > > > > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index > > 008bb34ee324..267b181fef15 100644 > > --- a/net/packet/af_packet.c > > +++ b/net/packet/af_packet.c > > @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk, > > union tpacket_req_u *req_u, > > case TPACKET_V3: > > /* Block transmit is not supported yet */ > > if (!tx_ring) { > > + prb_shutdown_retire_blk_timer(po, > > + rb_queue); > > init_prb_bdqc(po, rb, pg_vec, req_u); > > } else { > > struct tpacket_req3 *req3 = > > &req_u->req3;
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi Wang Cong, With this patch , the system was crashed when setsockopt. The call trace as below: crash> bt PID: 3069 TASK: 8800afcc CPU: 0 COMMAND: "trinity-main" #0 [8801bec03ce0] machine_kexec at 8105354b #1 [8801bec03d40] crash_kexec at 810f7e82 #2 [8801bec03e10] panic at 81650058 #3 [8801bec03e90] watchdog_timer_fn at 81122533 #4 [8801bec03ec8] __hrtimer_run_queues at 810abeb2 #5 [8801bec03f20] hrtimer_interrupt at 810ac450 #6 [8801bec03f70] local_apic_timer_interrupt at 8104a457 #7 [8801bec03f88] smp_apic_timer_interrupt at 8166aed0 #8 [8801bec03fb0] apic_timer_interrupt at 8166931d --- --- #9 [8801b301fcb8] apic_timer_interrupt at 8166931d [exception RIP: lock_timer_base+77] RIP: 8108dced RSP: 8801b301fd60 RFLAGS: 0246 RAX: RBX: 8800afcc RCX: 0001 RDX: 8800afcc RSI: 8801b301fd90 RDI: 8800b0d853c8 RBP: 8801b301fd80 R8: 8800afcc R9: ea000268 R10: 003c R11: 8801b301fb2e R12: 8800afcc R13: 8800afcc R14: R15: 83d1a340 ORIG_RAX: ff10 CS: 0010 SS: 0018 #10 [8801b301fd88] try_to_del_timer_sync at 8108f19f #11 [8801b301fdb8] del_timer_sync at 8108f252 #12 [8801b301fdd0] packet_set_ring at 81635e60 #13 [8801b301fe98] packet_setsockopt at 81636760 #14 [8801b301ff38] sys_setsockopt at 81531860 #15 [8801b301ff80] tracesys at 816687ed (via system_call) RIP: 7fcc78b03e3a RSP: 7fff16f246b8 RFLAGS: 0202 RAX: ffda RBX: 816687ed RCX: RDX: 0005 RSI: 0107 RDI: 0180 RBP: 0180 R8: 001c R9: 7fcc78dc7160 R10: 01fd6ba0 R11: 0202 R12: R13: 0011 R14: 01fd6b60 R15: 01fd6b70 ORIG_RAX: 0036 CS: 0033 SS: 002b Best Regards, liujian > -Original Message- > From: Cong Wang [mailto:xiyou.wangc...@gmail.com] > Sent: Sunday, July 23, 2017 1:59 PM > To: Dingtianhong > Cc: liujian (CE); Willem de Bruijn; Dave Jones; alexander.le...@verizon.com; > da...@davemloft.net; eduma...@google.com; will...@google.com; > dan...@iogearbox.net; netdev@vger.kernel.org; linux-ker...@vger.kernel.org > Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired > > On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong > wrote: > > Hi, Cong: > > > > Thanks for your quirk solution, but I still has some doubts about it, > > it looks like fix the problem in the > > packet_setsockopt->packet_set_ring processing, but when in > > packet_release processing, it may could not release the real pg_vec > > for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss > > something here, nice to hear from your feedback. :) > > Yes you miss that packet_release() has memset()'s so we won't hit that path. > :) > > However, I missed the swap() in this messy function, actually I believe the > bug > is that we modify tpacket_kbdq_core inside rx_ring in non-closing case without > actually stopping its timer. I feel more confident with the following patch: > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index > 008bb34ee324..267b181fef15 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk, union > tpacket_req_u *req_u, > case TPACKET_V3: > /* Block transmit is not supported yet */ > if (!tx_ring) { > + prb_shutdown_retire_blk_timer(po, > + rb_queue); > init_prb_bdqc(po, rb, pg_vec, req_u); > } else { > struct tpacket_req3 *req3 = > &req_u->req3;
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On Sat, Jul 22, 2017 at 8:40 PM, Ding Tianhong wrote: > Hi, Cong: > > Thanks for your quirk solution, but I still has some doubts about it, > it looks like fix the problem in the packet_setsockopt->packet_set_ring > processing, > but when in packet_release processing, it may could not release the > real pg_vec for the TPACKET_V3 ring, and then cause the mem leak, > maybe I miss something here, nice to hear from your feedback. :) Yes you miss that packet_release() has memset()'s so we won't hit that path. :) However, I missed the swap() in this messy function, actually I believe the bug is that we modify tpacket_kbdq_core inside rx_ring in non-closing case without actually stopping its timer. I feel more confident with the following patch: diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 008bb34ee324..267b181fef15 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4263,6 +4263,7 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u, case TPACKET_V3: /* Block transmit is not supported yet */ if (!tx_ring) { + prb_shutdown_retire_blk_timer(po, rb_queue); init_prb_bdqc(po, rb, pg_vec, req_u); } else { struct tpacket_req3 *req3 = &req_u->req3;
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On 2017/7/23 3:02, Cong Wang wrote: > Hello, > > On Sat, Jul 22, 2017 at 2:55 AM, liujian (CE) wrote: >> I also hit this issue with trinity test: >> >> The call trace: >> [exception RIP: prb_retire_rx_blk_timer_expired+70] >> RIP: 81633be6 RSP: 8801bec03dc0 RFLAGS: 00010246 >> RAX: RBX: 8801b49d0948 RCX: >> RDX: 8801b31057a0 RSI: a56b6b6b6b6b6b6b RDI: 8801b49d09ec >> RBP: 8801bec03dd8 R8: 0001 R9: 83e1bf80 >> R10: 0002 R11: 0005 R12: 8801b49d09ec >> R13: 0100 R14: 81633ba0 R15: 8801b49d0948 >> ORIG_RAX: CS: 0010 SS: 0018 >> #7 [8801bec03de0] call_timer_fn at 8108cb76 >> #8 [8801bec03e18] run_timer_softirq at 8108f87c >> #9 [8801bec03e90] __do_softirq at 8108629f >> #10 [8801bec03f00] call_softirq at 8166a01c >> #11 [8801bec03f18] do_softirq at 810172ad >> #12 [8801bec03f30] irq_exit at 81086655 >> #13 [8801bec03f48] msa_irq_exit at 810b1ab3 >> #14 [8801bec03f88] smp_apic_timer_interrupt at 8166aeae >> #15 [8801bec03fb0] apic_timer_interrupt at 816692dd >> --- --- >> >> And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); >> is a56b6b6b6b6b6b6b >> > > Does the following quick fix help? > > > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c > index 008bb34ee324..09ec1640e5f7 100644 > --- a/net/packet/af_packet.c > +++ b/net/packet/af_packet.c > @@ -4264,6 +4264,7 @@ static int packet_set_ring(struct sock *sk, > union tpacket_req_u *req_u, > /* Block transmit is not supported yet */ > if (!tx_ring) { > init_prb_bdqc(po, rb, pg_vec, req_u); > + pg_vec = NULL; > } else { > struct tpacket_req3 *req3 = &req_u->req3; > Hi, Cong: Thanks for your quirk solution, but I still has some doubts about it, it looks like fix the problem in the packet_setsockopt->packet_set_ring processing, but when in packet_release processing, it may could not release the real pg_vec for the TPACKET_V3 ring, and then cause the mem leak, maybe I miss something here, nice to hear from your feedback. :) what about fix it this way: --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4335,9 +4335,13 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u, /* Because we don't support block-based V3 on tx-ring */ if (!tx_ring) prb_shutdown_retire_blk_timer(po, rb_queue); + + if (pg_vec) + free_pg_vec(pg_vec, order, req->tp_block_nr); + } - if (pg_vec) + if (pg_vec && (po->tp_version < TPACKET_V3)) free_pg_vec(pg_vec, order, req->tp_block_nr); out: release_sock(sk); Regards Ding > . >
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
Hello, On Sat, Jul 22, 2017 at 2:55 AM, liujian (CE) wrote: > I also hit this issue with trinity test: > > The call trace: > [exception RIP: prb_retire_rx_blk_timer_expired+70] > RIP: 81633be6 RSP: 8801bec03dc0 RFLAGS: 00010246 > RAX: RBX: 8801b49d0948 RCX: > RDX: 8801b31057a0 RSI: a56b6b6b6b6b6b6b RDI: 8801b49d09ec > RBP: 8801bec03dd8 R8: 0001 R9: 83e1bf80 > R10: 0002 R11: 0005 R12: 8801b49d09ec > R13: 0100 R14: 81633ba0 R15: 8801b49d0948 > ORIG_RAX: CS: 0010 SS: 0018 > #7 [8801bec03de0] call_timer_fn at 8108cb76 > #8 [8801bec03e18] run_timer_softirq at 8108f87c > #9 [8801bec03e90] __do_softirq at 8108629f > #10 [8801bec03f00] call_softirq at 8166a01c > #11 [8801bec03f18] do_softirq at 810172ad > #12 [8801bec03f30] irq_exit at 81086655 > #13 [8801bec03f48] msa_irq_exit at 810b1ab3 > #14 [8801bec03f88] smp_apic_timer_interrupt at 8166aeae > #15 [8801bec03fb0] apic_timer_interrupt at 816692dd > --- --- > > And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); > is a56b6b6b6b6b6b6b > Does the following quick fix help? diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c index 008bb34ee324..09ec1640e5f7 100644 --- a/net/packet/af_packet.c +++ b/net/packet/af_packet.c @@ -4264,6 +4264,7 @@ static int packet_set_ring(struct sock *sk, union tpacket_req_u *req_u, /* Block transmit is not supported yet */ if (!tx_ring) { init_prb_bdqc(po, rb, pg_vec, req_u); + pg_vec = NULL; } else { struct tpacket_req3 *req3 = &req_u->req3;
RE: af_packet: use after free in prb_retire_rx_blk_timer_expired
I also hit this issue with trinity test: The call trace: [exception RIP: prb_retire_rx_blk_timer_expired+70] RIP: 81633be6 RSP: 8801bec03dc0 RFLAGS: 00010246 RAX: RBX: 8801b49d0948 RCX: RDX: 8801b31057a0 RSI: a56b6b6b6b6b6b6b RDI: 8801b49d09ec RBP: 8801bec03dd8 R8: 0001 R9: 83e1bf80 R10: 0002 R11: 0005 R12: 8801b49d09ec R13: 0100 R14: 81633ba0 R15: 8801b49d0948 ORIG_RAX: CS: 0010 SS: 0018 #7 [8801bec03de0] call_timer_fn at 8108cb76 #8 [8801bec03e18] run_timer_softirq at 8108f87c #9 [8801bec03e90] __do_softirq at 8108629f #10 [8801bec03f00] call_softirq at 8166a01c #11 [8801bec03f18] do_softirq at 810172ad #12 [8801bec03f30] irq_exit at 81086655 #13 [8801bec03f48] msa_irq_exit at 810b1ab3 #14 [8801bec03f88] smp_apic_timer_interrupt at 8166aeae #15 [8801bec03fb0] apic_timer_interrupt at 816692dd --- --- And from vmcore, I can see the pointer GET_CURR_PBLOCK_DESC_FROM_CORE(pkc); is a56b6b6b6b6b6b6b struct packet_ring_buffer rx_ring = { pg_vec = 0x0, head = 0x0, frames_per_block = 0x400, frame_size = 0x0, frame_max = 0x, pg_vec_order = 0x0, pg_vec_pages = 0x0, pg_vec_len = 0x0, pending_refcnt = 0x0, prb_bdqc = { pkbdq = 0x8801b31057a0, feature_req_word = 0x1, hdrlen = 0x44, reset_pending_on_curr_blk = 0x1, delete_blk_timer = 0x0, kactive_blk_num = 0x0, blk_sizeof_priv = 0x0, last_kactive_blk_num = 0x0, pkblk_start = 0x8800a700 struct: page excluded: kernel virtual address: 8800a700 type: "gdb_readmem_callback" struct: page excluded: kernel virtual address: 8800a700 type: "gdb_readmem_callback" , pkblk_end = 0x8800a720 "\002", kblk_size = 0x20, max_frame_len = 0x1fffd0, knum_blocks = 0x1, knxt_seq_num = 0x2, prev = 0x8800a730 struct: page excluded: kernel virtual address: 8800a730 type: "gdb_readmem_callback" struct: page excluded: kernel virtual address: 8800a730 type: "gdb_readmem_callback" , nxt_offset = 0x8800a730 struct: page excluded: kernel virtual address: 8800a730 type: "gdb_readmem_callback" struct: page excluded: kernel virtual address: 8800a730 type: "gdb_readmem_callback" , skb = 0x0, blk_fill_in_prog = { counter = 0x0 crash> struct pgv 0x8801b31057a0 struct pgv { buffer = 0xa56b6b6b6b6b6b6b } Best Regards, liujian > -Original Message- > From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] > On Behalf Of Willem de Bruijn > Sent: Wednesday, April 12, 2017 7:23 AM > To: Dave Jones; alexander.le...@verizon.com; da...@davemloft.net; > eduma...@google.com; will...@google.com; dan...@iogearbox.net; > netdev@vger.kernel.org; linux-ker...@vger.kernel.org > Subject: Re: af_packet: use after free in prb_retire_rx_blk_timer_expired > > On Mon, Apr 10, 2017 at 3:23 PM, Dave Jones > wrote: > > On Mon, Apr 10, 2017 at 07:03:30PM +, alexander.le...@verizon.com > wrote: > > > Hi all, > > > > > > I seem to be hitting this use-after-free on a -next kernel using trinity: > > > > > > [ 531.036054] BUG: KASAN: use-after-free in > > prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688) > > The retire_blk_timer is called after the pg_vec struct for this ring was > freed. > This should not happen. packet_set_ring stops the timer with del_timer_sync > when tearing down the ring before freeing that > struct: > > if (closing && (po->tp_version > TPACKET_V2)) { > /* Because we don't support block-based V3 on tx-ring */ > if (!tx_ring) > prb_shutdown_retire_blk_timer(po, rb_queue); > } > > if (pg_vec) > free_pg_vec(pg_vec, order, req->tp_block_nr); > > This is a similar race to the use-after-free fixed by 84ac7260236a > ("packet: fix race condition in packet_set_ring"). The previous race was > triggered by a call to setsockopt PACKET_VERSION changing tp_version while > the ring is active. It is not immediately obvious what is the cause now. I > suppose trinity does not give a trace of such system calls on this file > descriptor? > That would be helpful. > > The bug report shows both a timer firing after the packet_set_ring call that > freed the pg_vec, and later a CONFIG_DEBUG_OBJECTS_FREE warning that > the timer is still active when the socket is closed on release of the last > file > descriptor.
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On Mon, Apr 10, 2017 at 3:23 PM, Dave Jones wrote: > On Mon, Apr 10, 2017 at 07:03:30PM +, alexander.le...@verizon.com wrote: > > Hi all, > > > > I seem to be hitting this use-after-free on a -next kernel using trinity: > > > > [ 531.036054] BUG: KASAN: use-after-free in > prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688) The retire_blk_timer is called after the pg_vec struct for this ring was freed. This should not happen. packet_set_ring stops the timer with del_timer_sync when tearing down the ring before freeing that struct: if (closing && (po->tp_version > TPACKET_V2)) { /* Because we don't support block-based V3 on tx-ring */ if (!tx_ring) prb_shutdown_retire_blk_timer(po, rb_queue); } if (pg_vec) free_pg_vec(pg_vec, order, req->tp_block_nr); This is a similar race to the use-after-free fixed by 84ac7260236a ("packet: fix race condition in packet_set_ring"). The previous race was triggered by a call to setsockopt PACKET_VERSION changing tp_version while the ring is active. It is not immediately obvious what is the cause now. I suppose trinity does not give a trace of such system calls on this file descriptor? That would be helpful. The bug report shows both a timer firing after the packet_set_ring call that freed the pg_vec, and later a CONFIG_DEBUG_OBJECTS_FREE warning that the timer is still active when the socket is closed on release of the last file descriptor.
Re: af_packet: use after free in prb_retire_rx_blk_timer_expired
On Mon, Apr 10, 2017 at 07:03:30PM +, alexander.le...@verizon.com wrote: > Hi all, > > I seem to be hitting this use-after-free on a -next kernel using trinity: > > [ 531.036054] BUG: KASAN: use-after-free in prb_retire_rx_blk_timer_expired > (net/packet/af_packet.c:688) > [ 531.036961] Read of size 8 at addr 88038c1fb0e8 by task > swapper/1/0 > [ 531.037727] > > [ 531.037928] CPU: 1 PID: 0 Comm: swapper/1 Not > tainted 4.11.0-rc5-next-20170407-dirty #24 Funny, I was just going over my old pending bugs, and found this one from January that looks like what happens with the same bug, but without kasan.. context: PID: 0 TASK: 881ff2fa5100 CPU: 5 COMMAND: "swapper/5" panic: general protection fault: [#1] netversion: 2.2-1 (Feb 2014) Backtrace: #0 [881fffaa3c00] machine_kexec at 81044af8 #1 [881fffaa3c60] __crash_kexec at 810ec755 #2 [881fffaa3d28] crash_kexec at 810ec81f #3 [881fffaa3d40] oops_end at 8101e348 #4 [881fffaa3d68] die at 8101e76b #5 [881fffaa3d98] do_general_protection at 8101be76 #6 [881fffaa3dc0] general_protection at 817fe5a2 [exception RIP: prb_retire_rx_blk_timer_expired+65] RIP: 817e6e41 RSP: 881fffaa3e78 RFLAGS: 00010246 RAX: RBX: 881fd7075800 RCX: RDX: 883ff0a16bb0 RSI: 0074636361757063 RDI: 881fd70758bc RBP: 881fffaa3e88 R8: 0001 R9: 0005 R10: R11: R12: 881fd7075b78 R13: 0100 R14: 817e6e00 R15: 881fd7075800 ORIG_RAX: CS: 0010 SS: 0018 #7 [881fffaa3e90] call_timer_fn at 810cec35 #8 [881fffaa3ec8] run_timer_softirq at 810cf01c #9 [881fffaa3f28] __softirqentry_text_start at 817ff05c #10 [881fffaa3f88] irq_exit at 8107d5fc #11 [881fffaa3f98] smp_apic_timer_interrupt at 817feea2 #12 [881fffaa3fb0] apic_timer_interrupt at 817fd56f --- --- #13 [881ff2fbfdd0] apic_timer_interrupt at 817fd56f RIP: 0018 RSP: RFLAGS: 81ebbb60 RAX: e8e0002a0400 RBX: 0067b502e95f RCX: 0006 RDX: 002e RSI: 0034 RDI: 0001 RBP: 81150540 R8: 881ff2fbfee0 R9: 0001 R10: 0005 R11: 81ebbb60 R12: 881ff2fbfe48 R13: 881ff2fa5110 R14: R15: 881ff2fa5100 ORIG_RAX: 881fffab5340 CS: 20c49ba5e353f7cf SS: ff10 WARNING: possibly bogus exception frame Dmesg: Code: 00 00 48 8b 93 10 03 00 00 80 bb 21 03 00 00 00 44 0f b6 83 20 03 00 00 0f b7 c8 48 8b 34 ca 75 57 <44> 8b 5e 0c 45 85 db 74 1d 8b 93 68 03 00 00 85 d2 74 13 f3 90 RIP [] prb_retire_rx_blk_timer_expired+0x41/0x120 RSP [ cut here ]
af_packet: use after free in prb_retire_rx_blk_timer_expired
Hi all, I seem to be hitting this use-after-free on a -next kernel using trinity: [ 531.036054] BUG: KASAN: use-after-free in prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688) [ 531.036961] Read of size 8 at addr 88038c1fb0e8 by task swapper/1/0 [ 531.037727] [ 531.037928] CPU: 1 PID: 0 Comm: swapper/1 Not tainted 4.11.0-rc5-next-20170407-dirty #24 [ 531.038862] Call Trace: [ 531.039163] [ 531.039447] dump_stack (lib/dump_stack.c:54) [ 531.041612] print_address_description (mm/kasan/report.c:253) [ 531.042809] kasan_report (mm/kasan/report.c:352 mm/kasan/report.c:408) [ 531.043263] __asan_report_load8_noabort (mm/kasan/report.c:429) [ 531.043829] prb_retire_rx_blk_timer_expired (net/packet/af_packet.c:688) [ 531.048298] call_timer_fn.isra.15 (./arch/x86/include/asm/preempt.h:22 kernel/time/timer.c:1246) [ 531.048805] __run_timers (./include/linux/spinlock.h:324 kernel/time/timer.c:1308 kernel/time/timer.c:1601) [ 531.055404] run_timer_softirq (kernel/time/timer.c:1614) [ 531.055883] __do_softirq (./arch/x86/include/asm/preempt.h:22 kernel/softirq.c:286) [ 531.057507] irq_exit (kernel/softirq.c:364 kernel/softirq.c:405) [ 531.057893] smp_apic_timer_interrupt (arch/x86/kernel/apic/apic.c:965) [ 531.058446] apic_timer_interrupt (arch/x86/entry/entry_64.S:704) [ 531.058951] RIP: 0010:native_safe_halt (??:?) [ 531.059718] RSP: 0018:88039aa8fe88 EFLAGS: 0246 ORIG_RAX: ff10 [ 531.060593] RAX: 0008 RBX: 88039aa68fc0 RCX: [ 531.061411] RDX: 11007354d1f8 RSI: RDI: [ 531.062203] RBP: 88039aa8fe88 R08: 880376251bc0 R09: 0001 [ 531.063001] R10: 88038e0f7838 R11: 0001 R12: 88039aa68fc0 [ 531.064007] R13: 83df0028 R14: R15: 88039aa68fc0 [ 531.064811] [ 531.065886] default_idle (./arch/x86/include/asm/paravirt.h:98 arch/x86/kernel/process.c:341) [ 531.066284] arch_cpu_idle (arch/x86/kernel/process.c:333) [ 531.066692] default_idle_call (kernel/sched/idle.c:101) [ 531.067151] do_idle (kernel/sched/idle.c:156 kernel/sched/idle.c:245) [ 531.067537] cpu_startup_entry (kernel/sched/idle.c:350 (discriminator 1)) [ 531.067992] start_secondary (arch/x86/kernel/smpboot.c:276) [ 531.068444] secondary_startup_64 (arch/x86/kernel/verify_cpu.S:37) [ 531.068924] [ 531.069109] Allocated by task 18982: [ 531.069522] save_stack_trace (arch/x86/kernel/stacktrace.c:60) [ 531.069965] save_stack (mm/kasan/kasan.c:493 mm/kasan/kasan.c:514) [ 531.070347] kasan_kmalloc (mm/kasan/kasan.c:525 mm/kasan/kasan.c:617) [ 531.070757] __kmalloc (mm/slub.c:3747) [ 531.071153] packet_set_ring (net/packet/af_packet.c:4130 net/packet/af_packet.c:4218) [ 531.072024] packet_setsockopt (net/packet/af_packet.c:3617) [ 531.072525] SyS_setsockopt (net/socket.c:1797 net/socket.c:1777) [ 531.072968] do_syscall_64 (arch/x86/entry/common.c:284) [ 531.073405] return_from_SYSCALL_64 (arch/x86/entry/entry_64.S:249) [ 531.073893] [ 531.074076] Freed by task 7019: [ 531.074443] save_stack_trace (arch/x86/kernel/stacktrace.c:60) [ 531.074882] save_stack (mm/kasan/kasan.c:493 mm/kasan/kasan.c:514) [ 531.075275] kasan_slab_free (mm/kasan/kasan.c:525 mm/kasan/kasan.c:590) [ 531.075705] kfree (mm/slub.c:2966 mm/slub.c:3882) [ 531.076052] free_pg_vec (net/packet/af_packet.c:4096) [ 531.076448] packet_set_ring (net/packet/af_packet.c:4298) [ 531.076922] packet_setsockopt (net/packet/af_packet.c:3617) [ 531.077406] SyS_setsockopt (net/socket.c:1797 net/socket.c:1777) [ 531.077848] do_syscall_64 (arch/x86/entry/common.c:284) [ 531.078285] return_from_SYSCALL_64 (arch/x86/entry/entry_64.S:249) [ 531.078773] [ 531.078956] The buggy address belongs to the object at 88038c1fb0e8 [ 531.078956] which belongs to the cache kmalloc-8 of size 8 [ 531.080341] The buggy address is located 0 bytes inside of [ 531.080341] 8-byte region [88038c1fb0e8, 88038c1fb0f0) [ 531.081600] The buggy address belongs to the page: [ 531.082150] page:ea000e307e80 count:1 mapcount:0 mapping: (null) index:0x88038c1fbd90 compound_mapcount: 0 [ 531.083613] flags: 0x2fffc008100(slab|head) [ 531.084139] raw: 02fffc008100 88038c1fbd90 0