| Message ID | 20221208011122.2343363-9-jesse.brandeburg@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Michal Kubecek |
| Headers | show |
| Series | ethtool: clean up and fix | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Dec 07, 2022 at 05:11:17PM -0800, Jesse Brandeburg wrote: > The sanitizers[1] found a couple of things, but this change addresses > some bit shifts that cannot be contained by the target type. > > The mistake is that the code is using unsigned int a = (1 << N) all over > the place, but the appropriate way to code this is unsigned int an > assignment of (1UL << N) especially if N can ever be 31. > > Fix the most egregious of these problems by changing "1" to "1UL", as > per it would be if we had used the BIT() macro. > > [1] make CFLAGS+='-fsanitize=address,undefined' \ > LDFLAGS+='-lubsan -lasan' > > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > --- > amd8111e.c | 2 +- > internal.h | 4 ++-- > netlink/features.c | 4 ++-- > 3 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/amd8111e.c b/amd8111e.c > index 175516bd2904..5a2fc2082e55 100644 > --- a/amd8111e.c > +++ b/amd8111e.c > @@ -75,7 +75,7 @@ typedef enum { > }CMD3_BITS; > typedef enum { > > - INTR = (1 << 31), > + INTR = (1UL << 31), > PCSINT = (1 << 28), > LCINT = (1 << 27), > APINT5 = (1 << 26), While the signedness issue only directly affects only INTR value, I would rather prefer to keep the code consistent and fix the whole enum. Also, as you intend to introduce the BIT() macro in the series anyway, wouldn't it be cleaner to move this patch after the UAPI update and use BIT() instead? Michal
On 12/7/2022 10:34 PM, Michal Kubecek wrote: > On Wed, Dec 07, 2022 at 05:11:17PM -0800, Jesse Brandeburg wrote: >> - INTR = (1 << 31), >> + INTR = (1UL << 31), >> PCSINT = (1 << 28), >> LCINT = (1 << 27), >> APINT5 = (1 << 26), > > While the signedness issue only directly affects only INTR value, > I would rather prefer to keep the code consistent and fix the whole enum. > Also, as you intend to introduce the BIT() macro in the series anyway, > wouldn't it be cleaner to move this patch after the UAPI update and use > BIT() instead? I had done it this way to separate the "most minimal fix" from the "refactor", as I figure that is a clearer way to delineate changes. Also, this specifically addresses the issues found by the undefined behavior sanitizer. I'll do it whichever way you like, but you're correct, later in this series I fix up all the BIT() usages. Maybe we can just leave this patch as is, knowing the full fix comes during the refactor in 10/13 ?
On Fri, Dec 09, 2022 at 09:42:59AM -0800, Jesse Brandeburg wrote: > On 12/7/2022 10:34 PM, Michal Kubecek wrote: > > On Wed, Dec 07, 2022 at 05:11:17PM -0800, Jesse Brandeburg wrote: > > > > - INTR = (1 << 31), > > > + INTR = (1UL << 31), > > > PCSINT = (1 << 28), > > > LCINT = (1 << 27), > > > APINT5 = (1 << 26), > > > > While the signedness issue only directly affects only INTR value, > > I would rather prefer to keep the code consistent and fix the whole enum. > > Also, as you intend to introduce the BIT() macro in the series anyway, > > wouldn't it be cleaner to move this patch after the UAPI update and use > > BIT() instead? > > I had done it this way to separate the "most minimal fix" from the > "refactor", as I figure that is a clearer way to delineate changes. Also, > this specifically addresses the issues found by the undefined behavior > sanitizer. > > I'll do it whichever way you like, but you're correct, later in this series > I fix up all the BIT() usages. Maybe we can just leave this patch as is, > knowing the full fix comes during the refactor in 10/13 ? As we end up with BIT() everywhere anyway, I'm OK with either option, leaving this patch as it is or dropping it. When I was writing that comment, I had seen 09/13 (introduction of BIT()) but not 10/13 (refactoring everything to use it). Michal
On 12/9/2022 10:09 AM, Michal Kubecek wrote: >> I'll do it whichever way you like, but you're correct, later in this series >> I fix up all the BIT() usages. Maybe we can just leave this patch as is, >> knowing the full fix comes during the refactor in 10/13 ? > > As we end up with BIT() everywhere anyway, I'm OK with either option, > leaving this patch as it is or dropping it. When I was writing that > comment, I had seen 09/13 (introduction of BIT()) but not 10/13 > (refactoring everything to use it). Ok, thanks. Also Jakub pointed out to me there is a UAPI compliant _BITUL()/_BITULL() function in include/uapi/linux/const.h, which I'll switch 9 and 10 to using. Wish that function had been a tad more discoverable.
diff --git a/amd8111e.c b/amd8111e.c index 175516bd2904..5a2fc2082e55 100644 --- a/amd8111e.c +++ b/amd8111e.c @@ -75,7 +75,7 @@ typedef enum { }CMD3_BITS; typedef enum { - INTR = (1 << 31), + INTR = (1UL << 31), PCSINT = (1 << 28), LCINT = (1 << 27), APINT5 = (1 << 26), diff --git a/internal.h b/internal.h index dd7d6ac70ad4..6e79374bcfd5 100644 --- a/internal.h +++ b/internal.h @@ -205,14 +205,14 @@ static inline int ethtool_link_mode_test_bit(unsigned int nr, const u32 *mask) { if (nr >= ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBITS) return !!0; - return !!(mask[nr / 32] & (1 << (nr % 32))); + return !!(mask[nr / 32] & (1UL << (nr % 32))); } static inline int ethtool_link_mode_set_bit(unsigned int nr, u32 *mask) { if (nr >= ETHTOOL_LINK_MODE_MASK_MAX_KERNEL_NBITS) return -1; - mask[nr / 32] |= (1 << (nr % 32)); + mask[nr / 32] |= (1UL << (nr % 32)); return 0; } diff --git a/netlink/features.c b/netlink/features.c index a4dae8fac4dc..f6ba47f21a12 100644 --- a/netlink/features.c +++ b/netlink/features.c @@ -57,7 +57,7 @@ static int prepare_feature_results(const struct nlattr *const *tb, static bool feature_on(const uint32_t *bitmap, unsigned int idx) { - return bitmap[idx / 32] & (1 << (idx % 32)); + return bitmap[idx / 32] & (1UL << (idx % 32)); } static void dump_feature(const struct feature_results *results, @@ -302,7 +302,7 @@ static void set_sf_req_mask(struct nl_context *nlctx, unsigned int idx) { struct sfeatures_context *sfctx = nlctx->cmd_private; - sfctx->req_mask[idx / 32] |= (1 << (idx % 32)); + sfctx->req_mask[idx / 32] |= (1UL << (idx % 32)); } static int fill_legacy_flag(struct nl_context *nlctx, const char *flag_name,
The sanitizers[1] found a couple of things, but this change addresses some bit shifts that cannot be contained by the target type. The mistake is that the code is using unsigned int a = (1 << N) all over the place, but the appropriate way to code this is unsigned int an assignment of (1UL << N) especially if N can ever be 31. Fix the most egregious of these problems by changing "1" to "1UL", as per it would be if we had used the BIT() macro. [1] make CFLAGS+='-fsanitize=address,undefined' \ LDFLAGS+='-lubsan -lasan' Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> --- amd8111e.c | 2 +- internal.h | 4 ++-- netlink/features.c | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-)