| Message ID | 20190614082015.23336-2-toshiaki.makita1@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | Devmap fixes around memory and RCU | expand |
Toshiaki Makita <toshiaki.makita1@gmail.com> writes: > dev_map_free() waits for flush_needed bitmap to be empty in order to > ensure all flush operations have completed before freeing its entries. > However the corresponding clear_bit() was called before using the > entries, so the entries could be used after free. > > All access to the entries needs to be done before clearing the bit. > It seems commit a5e2da6e9787 ("bpf: netdev is never null in > __dev_map_flush") accidentally changed the clear_bit() and memory access > order. > > Note that the problem happens only in __dev_map_flush(), not in > dev_map_flush_old(). dev_map_flush_old() is called only after nulling > out the corresponding netdev_map entry, so dev_map_free() never frees > the entry thus no such race happens there. > > Fixes: a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") > Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com> I recently posted a patch[0] that gets rid of the bitmap entirely, so I think you can drop this one... -Toke [0] https://lore.kernel.org/netdev/156042464148.25684.11881534392137955942.stgit@alrua-x1/
Toke Høiland-Jørgensen <toke@redhat.com> writes: > Toshiaki Makita <toshiaki.makita1@gmail.com> writes: > >> dev_map_free() waits for flush_needed bitmap to be empty in order to >> ensure all flush operations have completed before freeing its entries. >> However the corresponding clear_bit() was called before using the >> entries, so the entries could be used after free. >> >> All access to the entries needs to be done before clearing the bit. >> It seems commit a5e2da6e9787 ("bpf: netdev is never null in >> __dev_map_flush") accidentally changed the clear_bit() and memory access >> order. >> >> Note that the problem happens only in __dev_map_flush(), not in >> dev_map_flush_old(). dev_map_flush_old() is called only after nulling >> out the corresponding netdev_map entry, so dev_map_free() never frees >> the entry thus no such race happens there. >> >> Fixes: a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") >> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com> > > I recently posted a patch[0] that gets rid of the bitmap entirely, so I > think you can drop this one... Alternatively, since this entire series should probably go to stable, I can respin mine on top of it? -Toke
On Fri, 14 Jun 2019 13:04:53 +0200 Toke Høiland-Jørgensen <toke@redhat.com> wrote: > Toshiaki Makita <toshiaki.makita1@gmail.com> writes: > > > dev_map_free() waits for flush_needed bitmap to be empty in order to > > ensure all flush operations have completed before freeing its entries. > > However the corresponding clear_bit() was called before using the > > entries, so the entries could be used after free. > > > > All access to the entries needs to be done before clearing the bit. > > It seems commit a5e2da6e9787 ("bpf: netdev is never null in > > __dev_map_flush") accidentally changed the clear_bit() and memory access > > order. > > > > Note that the problem happens only in __dev_map_flush(), not in > > dev_map_flush_old(). dev_map_flush_old() is called only after nulling > > out the corresponding netdev_map entry, so dev_map_free() never frees > > the entry thus no such race happens there. > > > > Fixes: a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") > > Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com> > > I recently posted a patch[0] that gets rid of the bitmap entirely, so I > think you can drop this one... One could argue that this is a stable tree fix... which unfortunately will cause some pain for your patch. Or maybe for the maintainers, as this is for 'bpf' git-tree and your patch is for 'bpf-next' git-tree. > [0] https://lore.kernel.org/netdev/156042464148.25684.11881534392137955942.stgit@alrua-x1/
On 19/06/14 (金) 21:10:38, Toke Høiland-Jørgensen wrote: > Toke Høiland-Jørgensen <toke@redhat.com> writes: > >> Toshiaki Makita <toshiaki.makita1@gmail.com> writes: >> >>> dev_map_free() waits for flush_needed bitmap to be empty in order to >>> ensure all flush operations have completed before freeing its entries. >>> However the corresponding clear_bit() was called before using the >>> entries, so the entries could be used after free. >>> >>> All access to the entries needs to be done before clearing the bit. >>> It seems commit a5e2da6e9787 ("bpf: netdev is never null in >>> __dev_map_flush") accidentally changed the clear_bit() and memory access >>> order. >>> >>> Note that the problem happens only in __dev_map_flush(), not in >>> dev_map_flush_old(). dev_map_flush_old() is called only after nulling >>> out the corresponding netdev_map entry, so dev_map_free() never frees >>> the entry thus no such race happens there. >>> >>> Fixes: a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") >>> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com> >> >> I recently posted a patch[0] that gets rid of the bitmap entirely, so I >> think you can drop this one... > > Alternatively, since this entire series should probably go to stable, I > can respin mine on top of it? Indeed conflict will happen, as this is for 'bpf' not 'bpf-next'. Sorry for disturbing your work. I'm also not sure how to proceed in this case. Toshiaki Makita
Toshiaki Makita <toshiaki.makita1@gmail.com> writes: > On 19/06/14 (金) 21:10:38, Toke Høiland-Jørgensen wrote: >> Toke Høiland-Jørgensen <toke@redhat.com> writes: >> >>> Toshiaki Makita <toshiaki.makita1@gmail.com> writes: >>> >>>> dev_map_free() waits for flush_needed bitmap to be empty in order to >>>> ensure all flush operations have completed before freeing its entries. >>>> However the corresponding clear_bit() was called before using the >>>> entries, so the entries could be used after free. >>>> >>>> All access to the entries needs to be done before clearing the bit. >>>> It seems commit a5e2da6e9787 ("bpf: netdev is never null in >>>> __dev_map_flush") accidentally changed the clear_bit() and memory access >>>> order. >>>> >>>> Note that the problem happens only in __dev_map_flush(), not in >>>> dev_map_flush_old(). dev_map_flush_old() is called only after nulling >>>> out the corresponding netdev_map entry, so dev_map_free() never frees >>>> the entry thus no such race happens there. >>>> >>>> Fixes: a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") >>>> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com> >>> >>> I recently posted a patch[0] that gets rid of the bitmap entirely, so I >>> think you can drop this one... >> >> Alternatively, since this entire series should probably go to stable, I >> can respin mine on top of it? > > Indeed conflict will happen, as this is for 'bpf' not 'bpf-next'. > Sorry for disturbing your work. Oh, no worries! > I'm also not sure how to proceed in this case. I guess we'll leave that up to the maintainers :) -Toke
On 06/14/2019 03:09 PM, Toke Høiland-Jørgensen wrote: > Toshiaki Makita <toshiaki.makita1@gmail.com> writes: [...] >>> Alternatively, since this entire series should probably go to stable, I >>> can respin mine on top of it? >> >> Indeed conflict will happen, as this is for 'bpf' not 'bpf-next'. >> Sorry for disturbing your work. > > Oh, no worries! > >> I'm also not sure how to proceed in this case. > > I guess we'll leave that up to the maintainers :) So all three look good to me, I've applied them to bpf tree. Fixes to bpf do have precedence over patches to bpf-next given they need to land in the current release. I'll get bpf out later tonight and ask David to merge net into net-next after that since rebase is also needed for Stanislav's cgroup series. We'll then flush out bpf-next so we can fast-fwd to net-next to pull in all the dependencies. Thanks a lot, Daniel
Daniel Borkmann <daniel@iogearbox.net> writes: > On 06/14/2019 03:09 PM, Toke Høiland-Jørgensen wrote: >> Toshiaki Makita <toshiaki.makita1@gmail.com> writes: > [...] >>>> Alternatively, since this entire series should probably go to stable, I >>>> can respin mine on top of it? >>> >>> Indeed conflict will happen, as this is for 'bpf' not 'bpf-next'. >>> Sorry for disturbing your work. >> >> Oh, no worries! >> >>> I'm also not sure how to proceed in this case. >> >> I guess we'll leave that up to the maintainers :) > > So all three look good to me, I've applied them to bpf tree. Fixes to > bpf do have precedence over patches to bpf-next given they need to > land in the current release. I'll get bpf out later tonight and ask > David to merge net into net-next after that since rebase is also > needed for Stanislav's cgroup series. We'll then flush out bpf-next so > we can fast-fwd to net-next to pull in all the dependencies. Right, I'll wait for that, then rebase my series and resubmit -Toke
diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c index 1e525d7..e001fb1 100644 --- a/kernel/bpf/devmap.c +++ b/kernel/bpf/devmap.c @@ -291,10 +291,10 @@ void __dev_map_flush(struct bpf_map *map) if (unlikely(!dev)) continue; - __clear_bit(bit, bitmap); - bq = this_cpu_ptr(dev->bulkq); bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, true); + + __clear_bit(bit, bitmap); } }
dev_map_free() waits for flush_needed bitmap to be empty in order to ensure all flush operations have completed before freeing its entries. However the corresponding clear_bit() was called before using the entries, so the entries could be used after free. All access to the entries needs to be done before clearing the bit. It seems commit a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") accidentally changed the clear_bit() and memory access order. Note that the problem happens only in __dev_map_flush(), not in dev_map_flush_old(). dev_map_flush_old() is called only after nulling out the corresponding netdev_map entry, so dev_map_free() never frees the entry thus no such race happens there. Fixes: a5e2da6e9787 ("bpf: netdev is never null in __dev_map_flush") Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com> --- kernel/bpf/devmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)