diff mbox series

[net-next,v4,2/5] bitops: add non-atomic bitops for pointers

Message ID 20210628114746.129669-3-memxor@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series Generic XDP improvements | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 8 maintainers not CCed: yhs@fb.com akpm@linux-foundation.org kpsingh@kernel.org andriy.shevchenko@linux.intel.com geert+renesas@glider.be songliubraving@fb.com richard.weiyang@linux.alibaba.com yury.norov@gmail.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 19111 this patch: 19111
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 45 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 18576 this patch: 18576
netdev/header_inline success Link

Commit Message

Kumar Kartikeya Dwivedi June 28, 2021, 11:47 a.m. UTC
cpumap needs to set, clear, and test the lowest bit in skb pointer in
various places. To make these checks less noisy, add pointer friendly
bitop macros that also do some typechecking to sanitize the argument.

These wrap the non-atomic bitops __set_bit, __clear_bit, and test_bit
but for pointer arguments. Pointer's address has to be passed in and it
is treated as an unsigned long *, since width and representation of
pointer and unsigned long match on targets Linux supports. They are
prefixed with double underscore to indicate lack of atomicity.

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bitops.h    | 19 +++++++++++++++++++
 include/linux/typecheck.h | 10 ++++++++++
 2 files changed, 29 insertions(+)

Comments

Alexei Starovoitov June 29, 2021, 1:32 a.m. UTC | #1
On Mon, Jun 28, 2021 at 05:17:43PM +0530, Kumar Kartikeya Dwivedi wrote:
> cpumap needs to set, clear, and test the lowest bit in skb pointer in
> various places. To make these checks less noisy, add pointer friendly
> bitop macros that also do some typechecking to sanitize the argument.
> 
> These wrap the non-atomic bitops __set_bit, __clear_bit, and test_bit
> but for pointer arguments. Pointer's address has to be passed in and it
> is treated as an unsigned long *, since width and representation of
> pointer and unsigned long match on targets Linux supports. They are
> prefixed with double underscore to indicate lack of atomicity.
> 
> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/bitops.h    | 19 +++++++++++++++++++
>  include/linux/typecheck.h | 10 ++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 26bf15e6cd35..a9e336b9fa4d 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -4,6 +4,7 @@
>  
>  #include <asm/types.h>
>  #include <linux/bits.h>
> +#include <linux/typecheck.h>
>  
>  #include <uapi/linux/kernel.h>
>  
> @@ -253,6 +254,24 @@ static __always_inline void __assign_bit(long nr, volatile unsigned long *addr,
>  		__clear_bit(nr, addr);
>  }
>  
> +#define __ptr_set_bit(nr, addr)                         \
> +	({                                              \
> +		typecheck_pointer(*(addr));             \
> +		__set_bit(nr, (unsigned long *)(addr)); \
> +	})
> +
> +#define __ptr_clear_bit(nr, addr)                         \
> +	({                                                \
> +		typecheck_pointer(*(addr));               \
> +		__clear_bit(nr, (unsigned long *)(addr)); \
> +	})
> +
> +#define __ptr_test_bit(nr, addr)                       \
> +	({                                             \
> +		typecheck_pointer(*(addr));            \
> +		test_bit(nr, (unsigned long *)(addr)); \
> +	})

The use case is to use lower bits of pointers to store extra data, right?
The kernel is full of such tricks, so it's nice to formalize
the accessors, but the new macros need a comment and example
in this file.

> +
>  #ifdef __KERNEL__
>  
>  #ifndef set_mask_bits
> diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
> index 20d310331eb5..33c78f27147a 100644
> --- a/include/linux/typecheck.h
> +++ b/include/linux/typecheck.h
> @@ -22,4 +22,14 @@
>  	(void)__tmp; \
>  })
>  
> +/*
> + * Check at compile that something is a pointer type.

'at compile time'.

> + * Always evaluates to 1 so you may use it easily in comparisons.

I would drop this sentence.
The copy-paste from typecheck() macro is making it too verbose. imo.
Kinda obvious what it does.

> + */
> +#define typecheck_pointer(x) \
> +({	typeof(x) __dummy; \
> +	(void)sizeof(*__dummy); \
> +	1; \
> +})
> +
>  #endif		/* TYPECHECK_H_INCLUDED */
> -- 
> 2.31.1
>
diff mbox series

Patch

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 26bf15e6cd35..a9e336b9fa4d 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -4,6 +4,7 @@ 
 
 #include <asm/types.h>
 #include <linux/bits.h>
+#include <linux/typecheck.h>
 
 #include <uapi/linux/kernel.h>
 
@@ -253,6 +254,24 @@  static __always_inline void __assign_bit(long nr, volatile unsigned long *addr,
 		__clear_bit(nr, addr);
 }
 
+#define __ptr_set_bit(nr, addr)                         \
+	({                                              \
+		typecheck_pointer(*(addr));             \
+		__set_bit(nr, (unsigned long *)(addr)); \
+	})
+
+#define __ptr_clear_bit(nr, addr)                         \
+	({                                                \
+		typecheck_pointer(*(addr));               \
+		__clear_bit(nr, (unsigned long *)(addr)); \
+	})
+
+#define __ptr_test_bit(nr, addr)                       \
+	({                                             \
+		typecheck_pointer(*(addr));            \
+		test_bit(nr, (unsigned long *)(addr)); \
+	})
+
 #ifdef __KERNEL__
 
 #ifndef set_mask_bits
diff --git a/include/linux/typecheck.h b/include/linux/typecheck.h
index 20d310331eb5..33c78f27147a 100644
--- a/include/linux/typecheck.h
+++ b/include/linux/typecheck.h
@@ -22,4 +22,14 @@ 
 	(void)__tmp; \
 })
 
+/*
+ * Check at compile that something is a pointer type.
+ * Always evaluates to 1 so you may use it easily in comparisons.
+ */
+#define typecheck_pointer(x) \
+({	typeof(x) __dummy; \
+	(void)sizeof(*__dummy); \
+	1; \
+})
+
 #endif		/* TYPECHECK_H_INCLUDED */