* [PATCH v2 0/2 net] bitfield.h cleanups
@ 2020-07-08 23:04 Nick Desaulniers
2020-07-08 23:04 ` [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT Nick Desaulniers
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Nick Desaulniers @ 2020-07-08 23:04 UTC (permalink / raw)
To: David S . Miller
Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh,
oss-drivers, netdev, linux-kernel, Jakub Kicinski, Alex Elder,
Nick Desaulniers
Two patches, one that removes a BUILD_BUG_ON for a case that is not a
compile time bug (exposed by compiler optimization).
The second is a general cleanup in the area.
I decided to leave the BUILD_BUG_ON case first, as I hope it will
simplify being able to backport it to stable, and because I don't think
there's any type promotion+conversion bugs there.
Though it would be nice to use consistent types widths and signedness,
equality against literal zero is not an issue.
Jakub Kicinski (1):
bitfield.h: don't compile-time validate _val in FIELD_FIT
Nick Desaulniers (1):
bitfield.h: split up __BF_FIELD_CHECK macro
.../netronome/nfp/nfpcore/nfp_nsp_eth.c | 11 ++++----
include/linux/bitfield.h | 26 +++++++++++++------
2 files changed, 24 insertions(+), 13 deletions(-)
--
2.27.0.383.g050319c2ae-goog
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT 2020-07-08 23:04 [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers @ 2020-07-08 23:04 ` Nick Desaulniers 2020-07-09 1:48 ` Alex Elder 2020-07-08 23:04 ` [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro Nick Desaulniers 2020-07-14 18:23 ` [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers 2 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2020-07-08 23:04 UTC (permalink / raw) To: David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, netdev, linux-kernel, Jakub Kicinski, Alex Elder, stable, Masahiro Yamada, Sami Tolvanen, Nick Desaulniers From: Jakub Kicinski <kuba@kernel.org> When ur_load_imm_any() is inlined into jeq_imm(), it's possible for the compiler to deduce a case where _val can only have the value of -1 at compile time. Specifically, /* struct bpf_insn: _s32 imm */ u64 imm = insn->imm; /* sign extend */ if (imm >> 32) { /* non-zero only if insn->imm is negative */ /* inlined from ur_load_imm_any */ u32 __imm = imm >> 32; /* therefore, always 0xffffffff */ if (__builtin_constant_p(__imm) && __imm > 255) compiletime_assert_XXX() This can result in tripping a BUILD_BUG_ON() in __BF_FIELD_CHECK() that checks that a given value is representable in one byte (interpreted as unsigned). FIELD_FIT() should return true or false at runtime for whether a value can fit for not. Don't break the build over a value that's too large for the mask. We'd prefer to keep the inlining and compiler optimizations though we know this case will always return false. Cc: stable@vger.kernel.org Link: https://lore.kernel.org/kernel-hardening/CAK7LNASvb0UDJ0U5wkYYRzTAdnEs64HjXpEUL7d=V0CXiAXcNw@mail.gmail.com/ Reported-by: Masahiro Yamada <masahiroy@kernel.org> Debugged-by: Sami Tolvanen <samitolvanen@google.com> Signed-off-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes V1->V2: * None include/linux/bitfield.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 48ea093ff04c..4e035aca6f7e 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -77,7 +77,7 @@ */ #define FIELD_FIT(_mask, _val) \ ({ \ - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: "); \ + __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \ !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \ }) -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT 2020-07-08 23:04 ` [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT Nick Desaulniers @ 2020-07-09 1:48 ` Alex Elder 0 siblings, 0 replies; 13+ messages in thread From: Alex Elder @ 2020-07-09 1:48 UTC (permalink / raw) To: Nick Desaulniers, David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, netdev, linux-kernel, Jakub Kicinski, stable, Masahiro Yamada, Sami Tolvanen On 7/8/20 6:04 PM, Nick Desaulniers wrote: > From: Jakub Kicinski <kuba@kernel.org> > > When ur_load_imm_any() is inlined into jeq_imm(), it's possible for the > compiler to deduce a case where _val can only have the value of -1 at > compile time. Specifically, > > /* struct bpf_insn: _s32 imm */ > u64 imm = insn->imm; /* sign extend */ > if (imm >> 32) { /* non-zero only if insn->imm is negative */ > /* inlined from ur_load_imm_any */ > u32 __imm = imm >> 32; /* therefore, always 0xffffffff */ > if (__builtin_constant_p(__imm) && __imm > 255) > compiletime_assert_XXX() > > This can result in tripping a BUILD_BUG_ON() in __BF_FIELD_CHECK() that > checks that a given value is representable in one byte (interpreted as > unsigned). Looking at the 12 other callers of FIELD_FIT(), it's hard to tell but it appears most of them don't supply constant _val to the macro. So maybe relaxing this check is no great loss. I feel like I need to look much more deeply into this to call it a review, so: Acked-by: Alex Elder <elder@linaro.org> > FIELD_FIT() should return true or false at runtime for whether a value > can fit for not. Don't break the build over a value that's too large for > the mask. We'd prefer to keep the inlining and compiler optimizations > though we know this case will always return false. > > Cc: stable@vger.kernel.org > Link: https://lore.kernel.org/kernel-hardening/CAK7LNASvb0UDJ0U5wkYYRzTAdnEs64HjXpEUL7d=V0CXiAXcNw@mail.gmail.com/ > Reported-by: Masahiro Yamada <masahiroy@kernel.org> > Debugged-by: Sami Tolvanen <samitolvanen@google.com> > Signed-off-by: Jakub Kicinski <kuba@kernel.org> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> > --- > Changes V1->V2: > * None > > include/linux/bitfield.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 48ea093ff04c..4e035aca6f7e 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -77,7 +77,7 @@ > */ > #define FIELD_FIT(_mask, _val) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_FIT: "); \ > + __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \ > !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \ > }) > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro 2020-07-08 23:04 [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers 2020-07-08 23:04 ` [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT Nick Desaulniers @ 2020-07-08 23:04 ` Nick Desaulniers 2020-07-09 2:11 ` Alex Elder 2020-07-09 2:11 ` Alex Elder 2020-07-14 18:23 ` [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers 2 siblings, 2 replies; 13+ messages in thread From: Nick Desaulniers @ 2020-07-08 23:04 UTC (permalink / raw) To: David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, netdev, linux-kernel, Jakub Kicinski, Alex Elder, Nick Desaulniers This macro has a few expansion sites that pass literal 0s as parameters. Split these up so that we do the precise checks where we care about them. Suggested-by: Alex Elder <elder@linaro.org> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> --- Changes V1-V2: * New patch in v2. * Rebased on 0001. .../netronome/nfp/nfpcore/nfp_nsp_eth.c | 11 ++++---- include/linux/bitfield.h | 26 +++++++++++++------ 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c index 311a5be25acb..938fc733fccb 100644 --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c @@ -492,11 +492,12 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, return 0; } -#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ - ({ \ - __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \ - nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ - val, ctrl_bit); \ +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ + ({ \ + __BF_FIELD_CHECK_MASK(mask, "NFP_ETH_SET_BIT_CONFIG: "); \ + __BF_FIELD_CHECK_VAL(mask, val, "NFP_ETH_SET_BIT_CONFIG: "); \ + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ + val, ctrl_bit); \ }) /** diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h index 4e035aca6f7e..79651867beb3 100644 --- a/include/linux/bitfield.h +++ b/include/linux/bitfield.h @@ -41,18 +41,26 @@ #define __bf_shf(x) (__builtin_ffsll(x) - 1) -#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ +#define __BF_FIELD_CHECK_MASK(_mask, _pfx) \ ({ \ BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ _pfx "mask is not constant"); \ BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ + __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ + (1ULL << __bf_shf(_mask))); \ + }) + +#define __BF_FIELD_CHECK_VAL(_mask, _val, _pfx) \ + ({ \ BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ _pfx "value too large for the field"); \ - BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ + }) + +#define __BF_FIELD_CHECK_REG(_mask, _reg, _pfx) \ + ({ \ + BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ULL, \ _pfx "type of reg too small for mask"); \ - __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ - (1ULL << __bf_shf(_mask))); \ }) /** @@ -64,7 +72,7 @@ */ #define FIELD_MAX(_mask) \ ({ \ - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \ + __BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: "); \ (typeof(_mask))((_mask) >> __bf_shf(_mask)); \ }) @@ -77,7 +85,7 @@ */ #define FIELD_FIT(_mask, _val) \ ({ \ - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \ + __BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: "); \ !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \ }) @@ -91,7 +99,8 @@ */ #define FIELD_PREP(_mask, _val) \ ({ \ - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ + __BF_FIELD_CHECK_MASK(_mask, "FIELD_PREP: "); \ + __BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_PREP: "); \ ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ }) @@ -105,7 +114,8 @@ */ #define FIELD_GET(_mask, _reg) \ ({ \ - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \ + __BF_FIELD_CHECK_MASK(_mask, "FIELD_GET: "); \ + __BF_FIELD_CHECK_REG(_mask, _reg, "FIELD_GET: "); \ (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ }) -- 2.27.0.383.g050319c2ae-goog ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro 2020-07-08 23:04 ` [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro Nick Desaulniers @ 2020-07-09 2:11 ` Alex Elder 2020-07-09 2:11 ` Alex Elder 1 sibling, 0 replies; 13+ messages in thread From: Alex Elder @ 2020-07-09 2:11 UTC (permalink / raw) To: Nick Desaulniers, David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, netdev, linux-kernel, Jakub Kicinski On 7/8/20 6:04 PM, Nick Desaulniers wrote: > This macro has a few expansion sites that pass literal 0s as parameters. > Split these up so that we do the precise checks where we care about > them. > > Suggested-by: Alex Elder <elder@linaro.org> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> I do like this better. It makes it more obvious when only the mask is being verified (in FIELD_FIT() for example). I also appreciate that you distinguished the register checks from the value checks. - field masks must be (2^x - 1) << y, x > 0, and constant - values must be representable within a field mask - registers must be able to represent the field mask Reviewed-by: Alex Elder <elder@linaro.org> > --- > Changes V1-V2: > * New patch in v2. > * Rebased on 0001. > > .../netronome/nfp/nfpcore/nfp_nsp_eth.c | 11 ++++---- > include/linux/bitfield.h | 26 +++++++++++++------ > 2 files changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > index 311a5be25acb..938fc733fccb 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > @@ -492,11 +492,12 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > return 0; > } > > -#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ > - ({ \ > - __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \ > - nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ > - val, ctrl_bit); \ > +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ > + ({ \ > + __BF_FIELD_CHECK_MASK(mask, "NFP_ETH_SET_BIT_CONFIG: "); \ > + __BF_FIELD_CHECK_VAL(mask, val, "NFP_ETH_SET_BIT_CONFIG: "); \ > + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ > + val, ctrl_bit); \ > }) > > /** > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 4e035aca6f7e..79651867beb3 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -41,18 +41,26 @@ > > #define __bf_shf(x) (__builtin_ffsll(x) - 1) > > -#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ > +#define __BF_FIELD_CHECK_MASK(_mask, _pfx) \ > ({ \ > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ > _pfx "mask is not constant"); \ > BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ > + __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ > + (1ULL << __bf_shf(_mask))); \ > + }) > + > +#define __BF_FIELD_CHECK_VAL(_mask, _val, _pfx) \ > + ({ \ > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ > _pfx "value too large for the field"); \ > - BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ > + }) > + > +#define __BF_FIELD_CHECK_REG(_mask, _reg, _pfx) \ > + ({ \ > + BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ULL, \ > _pfx "type of reg too small for mask"); \ > - __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ > - (1ULL << __bf_shf(_mask))); \ > }) > > /** > @@ -64,7 +72,7 @@ > */ > #define FIELD_MAX(_mask) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: "); \ > (typeof(_mask))((_mask) >> __bf_shf(_mask)); \ > }) > > @@ -77,7 +85,7 @@ > */ > #define FIELD_FIT(_mask, _val) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: "); \ > !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \ > }) > > @@ -91,7 +99,8 @@ > */ > #define FIELD_PREP(_mask, _val) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_PREP: "); \ > + __BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_PREP: "); \ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > @@ -105,7 +114,8 @@ > */ > #define FIELD_GET(_mask, _reg) \ > ({ \ > - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_GET: "); \ > + __BF_FIELD_CHECK_REG(_mask, _reg, "FIELD_GET: "); \ > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ > }) > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro 2020-07-08 23:04 ` [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro Nick Desaulniers 2020-07-09 2:11 ` Alex Elder @ 2020-07-09 2:11 ` Alex Elder 1 sibling, 0 replies; 13+ messages in thread From: Alex Elder @ 2020-07-09 2:11 UTC (permalink / raw) To: Nick Desaulniers, David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, netdev, linux-kernel, Jakub Kicinski On 7/8/20 6:04 PM, Nick Desaulniers wrote: > This macro has a few expansion sites that pass literal 0s as parameters. > Split these up so that we do the precise checks where we care about > them. > > Suggested-by: Alex Elder <elder@linaro.org> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com> I do like this better. It makes it more obvious when only the mask is being verified (in FIELD_FIT() for example). I also appreciate that you distinguished the register checks from the value checks. - field masks must be (2^x - 1) << y, x > 0, and constant - values must be representable within a field mask - registers must be able to represent the field mask Reviewed-by: Alex Elder <elder@linaro.org> > --- > Changes V1-V2: > * New patch in v2. > * Rebased on 0001. > > .../netronome/nfp/nfpcore/nfp_nsp_eth.c | 11 ++++---- > include/linux/bitfield.h | 26 +++++++++++++------ > 2 files changed, 24 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > index 311a5be25acb..938fc733fccb 100644 > --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c > @@ -492,11 +492,12 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx, > return 0; > } > > -#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ > - ({ \ > - __BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \ > - nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ > - val, ctrl_bit); \ > +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit) \ > + ({ \ > + __BF_FIELD_CHECK_MASK(mask, "NFP_ETH_SET_BIT_CONFIG: "); \ > + __BF_FIELD_CHECK_VAL(mask, val, "NFP_ETH_SET_BIT_CONFIG: "); \ > + nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \ > + val, ctrl_bit); \ > }) > > /** > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h > index 4e035aca6f7e..79651867beb3 100644 > --- a/include/linux/bitfield.h > +++ b/include/linux/bitfield.h > @@ -41,18 +41,26 @@ > > #define __bf_shf(x) (__builtin_ffsll(x) - 1) > > -#define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx) \ > +#define __BF_FIELD_CHECK_MASK(_mask, _pfx) \ > ({ \ > BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask), \ > _pfx "mask is not constant"); \ > BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero"); \ > + __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ > + (1ULL << __bf_shf(_mask))); \ > + }) > + > +#define __BF_FIELD_CHECK_VAL(_mask, _val, _pfx) \ > + ({ \ > BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ? \ > ~((_mask) >> __bf_shf(_mask)) & (_val) : 0, \ > _pfx "value too large for the field"); \ > - BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull, \ > + }) > + > +#define __BF_FIELD_CHECK_REG(_mask, _reg, _pfx) \ > + ({ \ > + BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ULL, \ > _pfx "type of reg too small for mask"); \ > - __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) + \ > - (1ULL << __bf_shf(_mask))); \ > }) > > /** > @@ -64,7 +72,7 @@ > */ > #define FIELD_MAX(_mask) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_MAX: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_MAX: "); \ > (typeof(_mask))((_mask) >> __bf_shf(_mask)); \ > }) > > @@ -77,7 +85,7 @@ > */ > #define FIELD_FIT(_mask, _val) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, 0ULL, "FIELD_FIT: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_FIT: "); \ > !((((typeof(_mask))_val) << __bf_shf(_mask)) & ~(_mask)); \ > }) > > @@ -91,7 +99,8 @@ > */ > #define FIELD_PREP(_mask, _val) \ > ({ \ > - __BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_PREP: "); \ > + __BF_FIELD_CHECK_VAL(_mask, _val, "FIELD_PREP: "); \ > ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask); \ > }) > > @@ -105,7 +114,8 @@ > */ > #define FIELD_GET(_mask, _reg) \ > ({ \ > - __BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: "); \ > + __BF_FIELD_CHECK_MASK(_mask, "FIELD_GET: "); \ > + __BF_FIELD_CHECK_REG(_mask, _reg, "FIELD_GET: "); \ > (typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask)); \ > }) > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-07-08 23:04 [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers 2020-07-08 23:04 ` [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT Nick Desaulniers 2020-07-08 23:04 ` [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro Nick Desaulniers @ 2020-07-14 18:23 ` Nick Desaulniers 2020-07-20 19:48 ` Nick Desaulniers 2 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2020-07-14 18:23 UTC (permalink / raw) To: David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, Network Development, LKML, Jakub Kicinski, Alex Elder On Wed, Jul 8, 2020 at 4:04 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > Two patches, one that removes a BUILD_BUG_ON for a case that is not a > compile time bug (exposed by compiler optimization). > > The second is a general cleanup in the area. > > I decided to leave the BUILD_BUG_ON case first, as I hope it will > simplify being able to backport it to stable, and because I don't think > there's any type promotion+conversion bugs there. > > Though it would be nice to use consistent types widths and signedness, > equality against literal zero is not an issue. > > Jakub Kicinski (1): > bitfield.h: don't compile-time validate _val in FIELD_FIT > > Nick Desaulniers (1): > bitfield.h: split up __BF_FIELD_CHECK macro > > .../netronome/nfp/nfpcore/nfp_nsp_eth.c | 11 ++++---- > include/linux/bitfield.h | 26 +++++++++++++------ > 2 files changed, 24 insertions(+), 13 deletions(-) > > -- > 2.27.0.383.g050319c2ae-goog > Hey David, when you have a chance, could you please consider picking up this series? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-07-14 18:23 ` [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers @ 2020-07-20 19:48 ` Nick Desaulniers 2020-07-20 23:34 ` David Miller 0 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2020-07-20 19:48 UTC (permalink / raw) To: David S . Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, Network Development, LKML, Jakub Kicinski, Alex Elder On Tue, Jul 14, 2020 at 11:23 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Wed, Jul 8, 2020 at 4:04 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > > > Two patches, one that removes a BUILD_BUG_ON for a case that is not a > > compile time bug (exposed by compiler optimization). > > > > The second is a general cleanup in the area. > > > > I decided to leave the BUILD_BUG_ON case first, as I hope it will > > simplify being able to backport it to stable, and because I don't think > > there's any type promotion+conversion bugs there. > > > > Though it would be nice to use consistent types widths and signedness, > > equality against literal zero is not an issue. > > > > Jakub Kicinski (1): > > bitfield.h: don't compile-time validate _val in FIELD_FIT > > > > Nick Desaulniers (1): > > bitfield.h: split up __BF_FIELD_CHECK macro > > > > .../netronome/nfp/nfpcore/nfp_nsp_eth.c | 11 ++++---- > > include/linux/bitfield.h | 26 +++++++++++++------ > > 2 files changed, 24 insertions(+), 13 deletions(-) > > > > -- > > 2.27.0.383.g050319c2ae-goog > > > > Hey David, when you have a chance, could you please consider picking > up this series? > -- Hi David, bumping for review. -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-07-20 19:48 ` Nick Desaulniers @ 2020-07-20 23:34 ` David Miller 2020-07-30 22:37 ` Nick Desaulniers 0 siblings, 1 reply; 13+ messages in thread From: David Miller @ 2020-07-20 23:34 UTC (permalink / raw) To: ndesaulniers Cc: ast, daniel, kafai, songliubraving, yhs, andriin, john.fastabend, kpsingh, oss-drivers, netdev, linux-kernel, kuba, elder From: Nick Desaulniers <ndesaulniers@google.com> Date: Mon, 20 Jul 2020 12:48:38 -0700 > Hi David, bumping for review. If it's not active in my networking patchwork you can't just bump your original submission like this. You have to submit your series entirely again. And if it is in patchwork, such "bumping" is %100 unnecessary. It's in my queue, it is not lost, and I will process it when I have the time. Thank you. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-07-20 23:34 ` David Miller @ 2020-07-30 22:37 ` Nick Desaulniers 2020-08-05 17:44 ` Nick Desaulniers 0 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2020-07-30 22:37 UTC (permalink / raw) To: David Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, Network Development, LKML, Jakub Kicinski, Alex Elder, Kees Cook, Sami Tolvanen On Mon, Jul 20, 2020 at 4:35 PM David Miller <davem@davemloft.net> wrote: > > From: Nick Desaulniers <ndesaulniers@google.com> > Date: Mon, 20 Jul 2020 12:48:38 -0700 > > > Hi David, bumping for review. > > If it's not active in my networking patchwork you can't just bump your original > submission like this. > > You have to submit your series entirely again. > > And if it is in patchwork, such "bumping" is %100 unnecessary. It's > in my queue, it is not lost, and I will process it when I have the > time. > > Thank you. Hi David, Sorry, I'm not familiar with your workflow. By patchwork, are you referring to patchwork.ozlabs.org? If so, I guess filtering on Delegate=davem (https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=&q=&archive=&delegate=34) is your queue? I don't see my patches in the above query, but I do see my series here: https://patchwork.ozlabs.org/project/netdev/list/?series=188486&state=* but they're marked "Not Applicable". What does that mean? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-07-30 22:37 ` Nick Desaulniers @ 2020-08-05 17:44 ` Nick Desaulniers 2020-08-06 18:13 ` Jakub Kicinski 0 siblings, 1 reply; 13+ messages in thread From: Nick Desaulniers @ 2020-08-05 17:44 UTC (permalink / raw) To: David Miller Cc: Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, Network Development, LKML, Jakub Kicinski, Alex Elder, Kees Cook, Sami Tolvanen On Thu, Jul 30, 2020 at 3:37 PM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Mon, Jul 20, 2020 at 4:35 PM David Miller <davem@davemloft.net> wrote: > > > > From: Nick Desaulniers <ndesaulniers@google.com> > > Date: Mon, 20 Jul 2020 12:48:38 -0700 > > > > > Hi David, bumping for review. > > > > If it's not active in my networking patchwork you can't just bump your original > > submission like this. > > > > You have to submit your series entirely again. > > > > And if it is in patchwork, such "bumping" is %100 unnecessary. It's > > in my queue, it is not lost, and I will process it when I have the > > time. > > > > Thank you. > > Hi David, > Sorry, I'm not familiar with your workflow. By patchwork, are you > referring to patchwork.ozlabs.org? If so, I guess filtering on > Delegate=davem > (https://patchwork.ozlabs.org/project/netdev/list/?series=&submitter=&state=&q=&archive=&delegate=34) > is your queue? I don't see my patches in the above query, but I do > see my series here: > https://patchwork.ozlabs.org/project/netdev/list/?series=188486&state=* > but they're marked "Not Applicable". What does that mean? Hi David, I read through https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#q-how-often-do-changes-from-these-trees-make-it-to-the-mainline-linus-tree and noticed http://vger.kernel.org/~davem/net-next.html. Since the merge window just opened, it sounds like I'll need to wait 2 weeks for it to close before resending? Is that correct? Based on: > IMPORTANT: Do not send new net-next content to netdev during the period during which net-next tree is closed. Then based on the next section in the doc, it sounds like I was missing which tree to put the patch in, in the subject? I believe these patches should target net-next (not net) since they're not addressing regressions from the most recent cycle. Do I have all that right? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-08-05 17:44 ` Nick Desaulniers @ 2020-08-06 18:13 ` Jakub Kicinski 2020-08-06 18:15 ` Jakub Kicinski 0 siblings, 1 reply; 13+ messages in thread From: Jakub Kicinski @ 2020-08-06 18:13 UTC (permalink / raw) To: Nick Desaulniers Cc: David Miller, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, Network Development, LKML, Alex Elder, Kees Cook, Sami Tolvanen On Wed, 5 Aug 2020 10:44:30 -0700 Nick Desaulniers wrote: > Hi David, > I read through https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#q-how-often-do-changes-from-these-trees-make-it-to-the-mainline-linus-tree > and noticed http://vger.kernel.org/~davem/net-next.html. Since the > merge window just opened, it sounds like I'll need to wait 2 weeks for > it to close before resending? Is that correct? Based on: > > > IMPORTANT: Do not send new net-next content to netdev during the period during which net-next tree is closed. > > Then based on the next section in the doc, it sounds like I was > missing which tree to put the patch in, in the subject? I believe > these patches should target net-next (not net) since they're not > addressing regressions from the most recent cycle. > > Do I have all that right? Nick, please repost the first patch only (to:dave, cc:netdev,..., subject "[PATCH net resend] ...") and I'm pretty sure Dave will re-consider it, it's a build fix after all. In any case reviewing a patch with a short explanation under the commit message on why it's resend is easier [1] for a maintainer to process than digging through conversations. [1] may be related to the use of patchwork on netdev ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 0/2 net] bitfield.h cleanups 2020-08-06 18:13 ` Jakub Kicinski @ 2020-08-06 18:15 ` Jakub Kicinski 0 siblings, 0 replies; 13+ messages in thread From: Jakub Kicinski @ 2020-08-06 18:15 UTC (permalink / raw) To: Nick Desaulniers Cc: David Miller, Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, oss-drivers, Network Development, LKML, Alex Elder, Kees Cook, Sami Tolvanen On Thu, 6 Aug 2020 11:13:58 -0700 Jakub Kicinski wrote: > please repost the first patch only To be clear the second patch may need to wait for net-next since it's refactoring. That's why I suggest only posting patch 1 now, i.e. the fix. ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2020-08-06 18:41 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-08 23:04 [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers 2020-07-08 23:04 ` [PATCH v2 1/2 net] bitfield.h: don't compile-time validate _val in FIELD_FIT Nick Desaulniers 2020-07-09 1:48 ` Alex Elder 2020-07-08 23:04 ` [PATCH v2 2/2] bitfield.h: split up __BF_FIELD_CHECK macro Nick Desaulniers 2020-07-09 2:11 ` Alex Elder 2020-07-09 2:11 ` Alex Elder 2020-07-14 18:23 ` [PATCH v2 0/2 net] bitfield.h cleanups Nick Desaulniers 2020-07-20 19:48 ` Nick Desaulniers 2020-07-20 23:34 ` David Miller 2020-07-30 22:37 ` Nick Desaulniers 2020-08-05 17:44 ` Nick Desaulniers 2020-08-06 18:13 ` Jakub Kicinski 2020-08-06 18:15 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).