xdp: add NULL pointer check in __xdp_return()

PATCH Superseded Taehee Yoo 4 participants 6 messages Jul 25, 2018 – Jul 26, 2018

Subsystems (3)

Networking Network drivers XDP

lore

Share & tools

lore

Taehee Yoo

author

2018-07-25 16:22 UTC #0 lore raw

PATCH v2 1/1 xdp: add NULL pointer check in __xdp_return()

rhashtable_lookup() can return NULL. so that NULL pointer check routine should be added.

Fixes 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")

Signed-off-by Taehee Yoo <[email protected]>

---
V2 : add WARN_ON_ONCE when xa is NULL.

 net/core/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/core/xdp.c +4 -1

index 9d1f220..786fdbe 100644
@@ -345,7 +345,10 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
 		rcu_read_lock();
 		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
 		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-		xa->zc_alloc->free(xa->zc_alloc, handle);
+		if (!xa)
+			WARN_ON_ONCE(1);
+		else
+			xa->zc_alloc->free(xa->zc_alloc, handle);
 		rcu_read_unlock();
 	default:
 		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */

Signature · 1 line

2.9.3

Martin KaFai Lau

Acked-by on v2 1/1

2018-07-25 22:04 UTC #1 lore raw

Quoted 5 lines · Taehee Yoo

rhashtable_lookup() can return NULL. so that NULL pointer check routine should be added.
Fixes 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
Signed-off-by Taehee Yoo <[email protected]>

Acked-by Martin KaFai Lau <[email protected]>

Quoted 25 lines

---
V2 : add WARN_ON_ONCE when xa is NULL.

 net/core/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

net/core/xdp.c +4 -1

index 9d1f220..786fdbe 100644
@@ -345,7 +345,10 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
 		rcu_read_lock();
 		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
 		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-		xa->zc_alloc->free(xa->zc_alloc, handle);
+		if (!xa)
+			WARN_ON_ONCE(1);
+		else
+			xa->zc_alloc->free(xa->zc_alloc, handle);
 		rcu_read_unlock();
 	default:
 		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */

Signature · 1 line

2.9.3

Jakub Kicinski

2018-07-26 03:26 UTC #2 lore raw

In reply to Taehee Yoo

Quoted 16 lines · Taehee Yoo

rhashtable_lookup() can return NULL. so that NULL pointer check routine should be added.
Fixes 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
Signed-off-by Taehee Yoo <[email protected]>
---
V2 : add WARN_ON_ONCE when xa is NULL.
 net/core/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/core/xdp.c +0 -0
index 9d1f220..786fdbe 100644

net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct, · line 349

Quoted 6 lines

net/core/xdp.c +2 -1

@@ -345,7 +345,10 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
 		rcu_read_lock();
 		/* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
 		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-		xa->zc_alloc->free(xa->zc_alloc, handle);
+		if (!xa)
+			WARN_ON_ONCE(1);

nit: is compiler smart enough to figure out the fast path here? WARN_ON_ONCE() has the nice side effect of wrapping the condition in unlikely(). It could save us both LoC and potentially cycles to do:

if (!WARN_ON_ONCE(!xa))

	xa->zc_alloc->free(xa->zc_alloc, handle);

Although it admittedly looks a bit awkward. I'm not sure if we have some form of assert (i.e. positive check) in tree :S

Quoted 5 lines

net/core/xdp.c +2 -0

+		else
+			xa->zc_alloc->free(xa->zc_alloc, handle);
 		rcu_read_unlock();
 	default:
 		/* Not possible, checked in xdp_rxq_info_reg_mem_model() */

Björn Töpel

2018-07-26 13:23 UTC #3 lore raw

Den tors 26 juli 2018 kl 04:14 skrev Jakub Kicinski <[email protected]>:

Quoted 29 lines · Taehee Yoo

Quoted 16 lines
rhashtable_lookup() can return NULL. so that NULL pointer check routine should be added.
Fixes 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
Signed-off-by Taehee Yoo <[email protected]>
---
V2 : add WARN_ON_ONCE when xa is NULL.
 net/core/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/core/xdp.c +0 -0
index 9d1f220..786fdbe 100644
net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct, · line 349
Quoted 6 lines
net/core/xdp.c +2 -1
@@ -345,7 +345,10 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
              rcu_read_lock();
              /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
              xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-             xa->zc_alloc->free(xa->zc_alloc, handle);
+             if (!xa)
+                     WARN_ON_ONCE(1);
nit: is compiler smart enough to figure out the fast path here? WARN_ON_ONCE() has the nice side effect of wrapping the condition in unlikely(). It could save us both LoC and potentially cycles to do:

Quoted 6 lines

if (!WARN_ON_ONCE(!xa))
        xa->zc_alloc->free(xa->zc_alloc, handle);
Although it admittedly looks a bit awkward. I'm not sure if we have some form of assert (i.e. positive check) in tree :S

I'm kind of in favor of this ^^^. Hopefully, Taehee is ok with another spin.

Björn

Quoted 5 lines

net/core/xdp.c +2 -0

+             else
+                     xa->zc_alloc->free(xa->zc_alloc, handle);
              rcu_read_unlock();
      default:
              /* Not possible, checked in xdp_rxq_info_reg_mem_model() */

Taehee Yoo

author

2018-07-26 15:22 UTC #4 lore raw

2018-07-26 21:07 GMT+09:00 Björn Töpel <[email protected]>:

Quoted 31 lines

Den tors 26 juli 2018 kl 04:14 skrev Jakub Kicinski <[email protected]>:
Quoted 29 lines · Taehee Yoo
Quoted 16 lines
rhashtable_lookup() can return NULL. so that NULL pointer check routine should be added.
Fixes 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
Signed-off-by Taehee Yoo <[email protected]>
---
V2 : add WARN_ON_ONCE when xa is NULL.
 net/core/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/core/xdp.c +0 -0
index 9d1f220..786fdbe 100644
net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct, · line 349
Quoted 6 lines
net/core/xdp.c +2 -1
@@ -345,7 +345,10 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
              rcu_read_lock();
              /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
              xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-             xa->zc_alloc->free(xa->zc_alloc, handle);
+             if (!xa)
+                     WARN_ON_ONCE(1);
nit: is compiler smart enough to figure out the fast path here? WARN_ON_ONCE() has the nice side effect of wrapping the condition in unlikely(). It could save us both LoC and potentially cycles to do:

Quoted 9 lines

Quoted 6 lines
if (!WARN_ON_ONCE(!xa))
        xa->zc_alloc->free(xa->zc_alloc, handle);
Although it admittedly looks a bit awkward. I'm not sure if we have some form of assert (i.e. positive check) in tree :S
I'm kind of in favor of this ^^^. Hopefully, Taehee is ok with another spin.

I like this code style and I think it has performance benefit. So I will send v3 patch!

Thanks!

Quoted 7 lines

Björn

Quoted 5 lines

net/core/xdp.c +2 -0

+             else
+                     xa->zc_alloc->free(xa->zc_alloc, handle);
              rcu_read_unlock();
      default:
              /* Not possible, checked in xdp_rxq_info_reg_mem_model() */

Taehee Yoo

author

2018-07-26 15:19 UTC #5 lore raw

In reply to Jakub Kicinski

2018-07-26 11:11 GMT+09:00 Jakub Kicinski <[email protected]>:

Quoted 28 lines · Taehee Yoo

Quoted 16 lines
rhashtable_lookup() can return NULL. so that NULL pointer check routine should be added.
Fixes 02b55e5657c3 ("xdp: add MEM_TYPE_ZERO_COPY")
Signed-off-by Taehee Yoo <[email protected]>
---
V2 : add WARN_ON_ONCE when xa is NULL.
 net/core/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/core/xdp.c +0 -0
index 9d1f220..786fdbe 100644
net/core/xdp.c static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct, · line 349
Quoted 6 lines
net/core/xdp.c +2 -1
@@ -345,7 +345,10 @@ static void __xdp_return(void *data, struct xdp_mem_info *mem, bool napi_direct,
              rcu_read_lock();
              /* mem->id is valid, checked in xdp_rxq_info_reg_mem_model() */
              xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
-             xa->zc_alloc->free(xa->zc_alloc, handle);
+             if (!xa)
+                     WARN_ON_ONCE(1);
nit: is compiler smart enough to figure out the fast path here? WARN_ON_ONCE() has the nice side effect of wrapping the condition in unlikely(). It could save us both LoC and potentially cycles to do:

Quoted 6 lines

if (!WARN_ON_ONCE(!xa))
        xa->zc_alloc->free(xa->zc_alloc, handle);
Although it admittedly looks a bit awkward. I'm not sure if we have some form of assert (i.e. positive check) in tree :S

Thank you for suggestion!
I like this code style and I think there is no problem because readers are familiar with this code style.

I will send v3 patch!
Thanks!

Quoted 5 lines

net/core/xdp.c +2 -0

+             else
+                     xa->zc_alloc->free(xa->zc_alloc, handle);
              rcu_read_unlock();
      default:
              /* Not possible, checked in xdp_rxq_info_reg_mem_model() */