| Message ID | 8f6c2fec-0ac4-1414-69da-462114a4481f@huawei.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
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) <liujian56@huawei.com> 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); > > >> > > > . >
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.levin@verizon.com; > davem@davemloft.net; edumazet@google.com; willemb@google.com; > daniel@iogearbox.net; netdev@vger.kernel.org; linux-kernel@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) <liujian56@huawei.com> 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); > > > > > > >> > > > > > > . > >
--- 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); >