BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH bpf 0/3] Devmap fixes around memory and RCU
@ 2019-06-14  8:20 Toshiaki Makita
  2019-06-14  8:20 ` [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map Toshiaki Makita
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Toshiaki Makita @ 2019-06-14  8:20 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

Patch1: Jesper showed concerns around devmap free and flush race when
    reviewing my XDP bulk TX patch set, and I think indeed there is a
    race. Patch1 fix it.

Patch2: While reviewing dev_map_free I found bulk queue was not freed.
    Patch2 fix it.

Patch3: Some days ago David Ahern reported suspicous RCU usage in
    virtnet_xdp_xmit(), but it seems no one posted an official fix
    patch. So I arraged the fix.

Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>

Toshiaki Makita (3):
  devmap: Fix premature entry free on destroying map
  devmap: Add missing bulk queue free
  devmap: Add missing RCU read lock on flush

 kernel/bpf/devmap.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14  8:20 [PATCH bpf 0/3] Devmap fixes around memory and RCU Toshiaki Makita
@ 2019-06-14  8:20 ` Toshiaki Makita
  2019-06-14 11:04   ` Toke Høiland-Jørgensen
  2019-06-14  8:20 ` [PATCH bpf 2/3] devmap: Add missing bulk queue free Toshiaki Makita
  2019-06-14  8:20 ` [PATCH bpf 3/3] devmap: Add missing RCU read lock on flush Toshiaki Makita
  2 siblings, 1 reply; 14+ messages in thread
From: Toshiaki Makita @ 2019-06-14  8:20 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

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(-)

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);
 	}
 }
 
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf 2/3] devmap: Add missing bulk queue free
  2019-06-14  8:20 [PATCH bpf 0/3] Devmap fixes around memory and RCU Toshiaki Makita
  2019-06-14  8:20 ` [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map Toshiaki Makita
@ 2019-06-14  8:20 ` Toshiaki Makita
  2019-06-14 11:58   ` Jesper Dangaard Brouer
  2019-06-14  8:20 ` [PATCH bpf 3/3] devmap: Add missing RCU read lock on flush Toshiaki Makita
  2 siblings, 1 reply; 14+ messages in thread
From: Toshiaki Makita @ 2019-06-14  8:20 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

dev_map_free() forgot to free bulk queue when freeing its entries.

Fixes: 5d053f9da431 ("bpf: devmap prepare xdp frames for bulking")
Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
---
 kernel/bpf/devmap.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index e001fb1..a126d95 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -186,6 +186,7 @@ static void dev_map_free(struct bpf_map *map)
 		if (!dev)
 			continue;
 
+		free_percpu(dev->bulkq);
 		dev_put(dev->dev);
 		kfree(dev);
 	}
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH bpf 3/3] devmap: Add missing RCU read lock on flush
  2019-06-14  8:20 [PATCH bpf 0/3] Devmap fixes around memory and RCU Toshiaki Makita
  2019-06-14  8:20 ` [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map Toshiaki Makita
  2019-06-14  8:20 ` [PATCH bpf 2/3] devmap: Add missing bulk queue free Toshiaki Makita
@ 2019-06-14  8:20 ` Toshiaki Makita
  2019-06-14 11:07   ` Toke Høiland-Jørgensen
  2 siblings, 1 reply; 14+ messages in thread
From: Toshiaki Makita @ 2019-06-14  8:20 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

.ndo_xdp_xmit() assumes it is called under RCU. For example virtio_net
uses RCU to detect it has setup the resources for tx. The assumption
accidentally broke when introducing bulk queue in devmap.

Fixes: 5d053f9da431 ("bpf: devmap prepare xdp frames for bulking")
Reported-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
---
 kernel/bpf/devmap.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
index a126d95..1defea4 100644
--- a/kernel/bpf/devmap.c
+++ b/kernel/bpf/devmap.c
@@ -282,6 +282,7 @@ void __dev_map_flush(struct bpf_map *map)
 	unsigned long *bitmap = this_cpu_ptr(dtab->flush_needed);
 	u32 bit;
 
+	rcu_read_lock();
 	for_each_set_bit(bit, bitmap, map->max_entries) {
 		struct bpf_dtab_netdev *dev = READ_ONCE(dtab->netdev_map[bit]);
 		struct xdp_bulk_queue *bq;
@@ -297,6 +298,7 @@ void __dev_map_flush(struct bpf_map *map)
 
 		__clear_bit(bit, bitmap);
 	}
+	rcu_read_unlock();
 }
 
 /* rcu_read_lock (from syscall and BPF contexts) ensures that if a delete and/or
@@ -389,6 +391,7 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
 
 		int cpu;
 
+		rcu_read_lock();
 		for_each_online_cpu(cpu) {
 			bitmap = per_cpu_ptr(dev->dtab->flush_needed, cpu);
 			__clear_bit(dev->bit, bitmap);
@@ -396,6 +399,7 @@ static void dev_map_flush_old(struct bpf_dtab_netdev *dev)
 			bq = per_cpu_ptr(dev->bulkq, cpu);
 			bq_xmit_all(dev, bq, XDP_XMIT_FLUSH, false);
 		}
+		rcu_read_unlock();
 	}
 }
 
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14  8:20 ` [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map Toshiaki Makita
@ 2019-06-14 11:04   ` Toke Høiland-Jørgensen
  2019-06-14 12:10     ` Toke Høiland-Jørgensen
  2019-06-14 12:20     ` Jesper Dangaard Brouer
  0 siblings, 2 replies; 14+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-14 11:04 UTC (permalink / raw)
  To: Toshiaki Makita, Alexei Starovoitov, Daniel Borkmann,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

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/

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 3/3] devmap: Add missing RCU read lock on flush
  2019-06-14  8:20 ` [PATCH bpf 3/3] devmap: Add missing RCU read lock on flush Toshiaki Makita
@ 2019-06-14 11:07   ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 14+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-14 11:07 UTC (permalink / raw)
  To: Toshiaki Makita, Alexei Starovoitov, Daniel Borkmann,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

Toshiaki Makita <toshiaki.makita1@gmail.com> writes:

> .ndo_xdp_xmit() assumes it is called under RCU. For example virtio_net
> uses RCU to detect it has setup the resources for tx. The assumption
> accidentally broke when introducing bulk queue in devmap.
>
> Fixes: 5d053f9da431 ("bpf: devmap prepare xdp frames for bulking")
> Reported-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
> ---

I think this is still needed, but the patch context is going to conflict
with the patch I linked above... I guess it's up to the maintainers to
decide which order to merge them in :)

-Toke

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 2/3] devmap: Add missing bulk queue free
  2019-06-14  8:20 ` [PATCH bpf 2/3] devmap: Add missing bulk queue free Toshiaki Makita
@ 2019-06-14 11:58   ` Jesper Dangaard Brouer
  2019-06-14 13:03     ` Toshiaki Makita
  0 siblings, 1 reply; 14+ messages in thread
From: Jesper Dangaard Brouer @ 2019-06-14 11:58 UTC (permalink / raw)
  To: Toshiaki Makita
  Cc: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend, netdev,
	xdp-newbies, bpf, Michael S. Tsirkin, Jason Wang, David Ahern,
	brouer

On Fri, 14 Jun 2019 17:20:14 +0900
Toshiaki Makita <toshiaki.makita1@gmail.com> wrote:

> dev_map_free() forgot to free bulk queue when freeing its entries.
> 
> Fixes: 5d053f9da431 ("bpf: devmap prepare xdp frames for bulking")
> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
> ---
>  kernel/bpf/devmap.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index e001fb1..a126d95 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -186,6 +186,7 @@ static void dev_map_free(struct bpf_map *map)
>  		if (!dev)
>  			continue;
>  
> +		free_percpu(dev->bulkq);
>  		dev_put(dev->dev);
>  		kfree(dev);
>  	}

Do we need to call need to call dev_map_flush_old() before
free_percpu(dev->bulkq) ?

Looking the code, I guess this is not needed as, above we are ensuring
all pending flush operations have completed.

Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14 11:04   ` Toke Høiland-Jørgensen
@ 2019-06-14 12:10     ` Toke Høiland-Jørgensen
  2019-06-14 12:59       ` Toshiaki Makita
  2019-06-14 12:20     ` Jesper Dangaard Brouer
  1 sibling, 1 reply; 14+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-14 12:10 UTC (permalink / raw)
  To: Toshiaki Makita, Alexei Starovoitov, Daniel Borkmann,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend
  Cc: Toshiaki Makita, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14 11:04   ` Toke Høiland-Jørgensen
  2019-06-14 12:10     ` Toke Høiland-Jørgensen
@ 2019-06-14 12:20     ` Jesper Dangaard Brouer
  1 sibling, 0 replies; 14+ messages in thread
From: Jesper Dangaard Brouer @ 2019-06-14 12:20 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen
  Cc: Toshiaki Makita, Alexei Starovoitov, Daniel Borkmann,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend, netdev, xdp-newbies, bpf, Michael S. Tsirkin,
	Jason Wang, David Ahern, brouer

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/

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14 12:10     ` Toke Høiland-Jørgensen
@ 2019-06-14 12:59       ` Toshiaki Makita
  2019-06-14 13:09         ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 14+ messages in thread
From: Toshiaki Makita @ 2019-06-14 12:59 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, Alexei Starovoitov,
	Daniel Borkmann, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer, John Fastabend
  Cc: netdev, xdp-newbies, bpf, Michael S. Tsirkin, Jason Wang, David Ahern

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 2/3] devmap: Add missing bulk queue free
  2019-06-14 11:58   ` Jesper Dangaard Brouer
@ 2019-06-14 13:03     ` Toshiaki Makita
  0 siblings, 0 replies; 14+ messages in thread
From: Toshiaki Makita @ 2019-06-14 13:03 UTC (permalink / raw)
  To: Jesper Dangaard Brouer
  Cc: Alexei Starovoitov, Daniel Borkmann, David S. Miller,
	Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend, netdev,
	xdp-newbies, bpf, Michael S. Tsirkin, Jason Wang, David Ahern

On 19/06/14 (金) 20:58:06, Jesper Dangaard Brouer wrote:
> On Fri, 14 Jun 2019 17:20:14 +0900
> Toshiaki Makita <toshiaki.makita1@gmail.com> wrote:
> 
>> dev_map_free() forgot to free bulk queue when freeing its entries.
>>
>> Fixes: 5d053f9da431 ("bpf: devmap prepare xdp frames for bulking")
>> Signed-off-by: Toshiaki Makita <toshiaki.makita1@gmail.com>
>> ---
>>   kernel/bpf/devmap.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
>> index e001fb1..a126d95 100644
>> --- a/kernel/bpf/devmap.c
>> +++ b/kernel/bpf/devmap.c
>> @@ -186,6 +186,7 @@ static void dev_map_free(struct bpf_map *map)
>>   		if (!dev)
>>   			continue;
>>   
>> +		free_percpu(dev->bulkq);
>>   		dev_put(dev->dev);
>>   		kfree(dev);
>>   	}
> 
> Do we need to call need to call dev_map_flush_old() before
> free_percpu(dev->bulkq) ?
> 
> Looking the code, I guess this is not needed as, above we are ensuring
> all pending flush operations have completed.

My understanding is the same. The code waits for NAPI to flush the 
queue, so no need for dev_map_flush_old().

> 
> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>

Thanks!

Toshiaki Makita

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14 12:59       ` Toshiaki Makita
@ 2019-06-14 13:09         ` Toke Høiland-Jørgensen
  2019-06-14 23:07           ` Daniel Borkmann
  0 siblings, 1 reply; 14+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-14 13:09 UTC (permalink / raw)
  To: Toshiaki Makita, Alexei Starovoitov, Daniel Borkmann,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend
  Cc: netdev, xdp-newbies, bpf, Michael S. Tsirkin, Jason Wang, David Ahern

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14 13:09         ` Toke Høiland-Jørgensen
@ 2019-06-14 23:07           ` Daniel Borkmann
  2019-06-15 10:10             ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 14+ messages in thread
From: Daniel Borkmann @ 2019-06-14 23:07 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, Toshiaki Makita,
	Alexei Starovoitov, David S. Miller, Jakub Kicinski,
	Jesper Dangaard Brouer, John Fastabend
  Cc: netdev, xdp-newbies, bpf, Michael S. Tsirkin, Jason Wang, David Ahern

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map
  2019-06-14 23:07           ` Daniel Borkmann
@ 2019-06-15 10:10             ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 14+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-15 10:10 UTC (permalink / raw)
  To: Daniel Borkmann, Toshiaki Makita, Alexei Starovoitov,
	David S. Miller, Jakub Kicinski, Jesper Dangaard Brouer,
	John Fastabend
  Cc: netdev, xdp-newbies, bpf, Michael S. Tsirkin, Jason Wang, David Ahern

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-14  8:20 [PATCH bpf 0/3] Devmap fixes around memory and RCU Toshiaki Makita
2019-06-14  8:20 ` [PATCH bpf 1/3] devmap: Fix premature entry free on destroying map Toshiaki Makita
2019-06-14 11:04   ` Toke Høiland-Jørgensen
2019-06-14 12:10     ` Toke Høiland-Jørgensen
2019-06-14 12:59       ` Toshiaki Makita
2019-06-14 13:09         ` Toke Høiland-Jørgensen
2019-06-14 23:07           ` Daniel Borkmann
2019-06-15 10:10             ` Toke Høiland-Jørgensen
2019-06-14 12:20     ` Jesper Dangaard Brouer
2019-06-14  8:20 ` [PATCH bpf 2/3] devmap: Add missing bulk queue free Toshiaki Makita
2019-06-14 11:58   ` Jesper Dangaard Brouer
2019-06-14 13:03     ` Toshiaki Makita
2019-06-14  8:20 ` [PATCH bpf 3/3] devmap: Add missing RCU read lock on flush Toshiaki Makita
2019-06-14 11:07   ` Toke Høiland-Jørgensen

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org bpf@archiver.kernel.org
	public-inbox-index bpf


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/ public-inbox