| Message ID | a14df0e5-74aa-42c9-a444-ba4c7d733364@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l2-subdev: Fix a 64bit bug | expand |
Hi Dan, Thanks for the patch. On Fri, Nov 03, 2023 at 09:34:25AM +0300, Dan Carpenter wrote: > The problem is this line here from subdev_do_ioctl(). > > client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; > > The "client_cap->capabilities" variable is a u64. The AND operation > is supposed to clear out the V4L2_SUBDEV_CLIENT_CAP_STREAMS flag. But > because it's a 32 bit variable it accidentally clears our the high 32 > bits as well. > > Currently we only use BIT(0) and none ofthe upper bits so this doesn't > affect runtime behavior. > > Fixes: f57fa2959244 ("media: v4l2-subdev: Add new ioctl for client capabilities") > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > --- > include/uapi/linux/v4l2-subdev.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > index 4a195b68f28f..21d149969119 100644 > --- a/include/uapi/linux/v4l2-subdev.h > +++ b/include/uapi/linux/v4l2-subdev.h > @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { > * set (which is the default), the 'stream' fields will be forced to 0 by the > * kernel. > */ > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS BIT_ULL(0) This is a UAPI header but BIT_ULL() is defined in kernel-only headers. So (1ULL << 0) ? uapi/linux/const.h also has _BITULL().
On Fri, Nov 03, 2023 at 07:00:01AM +0000, Sakari Ailus wrote: > Hi Dan, > > Thanks for the patch. > > On Fri, Nov 03, 2023 at 09:34:25AM +0300, Dan Carpenter wrote: > > The problem is this line here from subdev_do_ioctl(). > > > > client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; > > > > The "client_cap->capabilities" variable is a u64. The AND operation > > is supposed to clear out the V4L2_SUBDEV_CLIENT_CAP_STREAMS flag. But > > because it's a 32 bit variable it accidentally clears our the high 32 > > bits as well. > > > > Currently we only use BIT(0) and none ofthe upper bits so this doesn't > > affect runtime behavior. > > > > Fixes: f57fa2959244 ("media: v4l2-subdev: Add new ioctl for client capabilities") > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > --- > > include/uapi/linux/v4l2-subdev.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > index 4a195b68f28f..21d149969119 100644 > > --- a/include/uapi/linux/v4l2-subdev.h > > +++ b/include/uapi/linux/v4l2-subdev.h > > @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { > > * set (which is the default), the 'stream' fields will be forced to 0 by the > > * kernel. > > */ > > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS BIT_ULL(0) > > This is a UAPI header but BIT_ULL() is defined in kernel-only headers. > > So (1ULL << 0) ? > > uapi/linux/const.h also has _BITULL(). Let's just do 1ULL < 0. I'll resend. Is there an automated way I could have caught this? regards, dan carpenter
Hi Dan, On Fri, Nov 03, 2023 at 10:24:40AM +0300, Dan Carpenter wrote: > On Fri, Nov 03, 2023 at 07:00:01AM +0000, Sakari Ailus wrote: > > Hi Dan, > > > > Thanks for the patch. > > > > On Fri, Nov 03, 2023 at 09:34:25AM +0300, Dan Carpenter wrote: > > > The problem is this line here from subdev_do_ioctl(). > > > > > > client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; > > > > > > The "client_cap->capabilities" variable is a u64. The AND operation > > > is supposed to clear out the V4L2_SUBDEV_CLIENT_CAP_STREAMS flag. But > > > because it's a 32 bit variable it accidentally clears our the high 32 > > > bits as well. > > > > > > Currently we only use BIT(0) and none ofthe upper bits so this doesn't > > > affect runtime behavior. > > > > > > Fixes: f57fa2959244 ("media: v4l2-subdev: Add new ioctl for client capabilities") > > > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> > > > --- > > > include/uapi/linux/v4l2-subdev.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > index 4a195b68f28f..21d149969119 100644 > > > --- a/include/uapi/linux/v4l2-subdev.h > > > +++ b/include/uapi/linux/v4l2-subdev.h > > > @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { > > > * set (which is the default), the 'stream' fields will be forced to 0 by the > > > * kernel. > > > */ > > > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > > > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS BIT_ULL(0) > > > > This is a UAPI header but BIT_ULL() is defined in kernel-only headers. > > > > So (1ULL << 0) ? > > > > uapi/linux/const.h also has _BITULL(). > > Let's just do 1ULL < 0. I'll resend. Is there an automated way I could > have caught this? I don't know. :-) Remember to use shift left for bit definitions in UAPI headers?
On Fri, Nov 03, 2023 at 07:32:41AM +0000, Sakari Ailus wrote: > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > > index 4a195b68f28f..21d149969119 100644 > > > > --- a/include/uapi/linux/v4l2-subdev.h > > > > +++ b/include/uapi/linux/v4l2-subdev.h ^^^^ > > > > @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { > > > > * set (which is the default), the 'stream' fields will be forced to 0 by the > > > > * kernel. > > > > */ > > > > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > > > > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS BIT_ULL(0) > > > > > > This is a UAPI header but BIT_ULL() is defined in kernel-only headers. > > > > > > So (1ULL << 0) ? > > > > > > uapi/linux/const.h also has _BITULL(). > > > > Let's just do 1ULL < 0. I'll resend. Is there an automated way I could > > have caught this? > > I don't know. :-) Remember to use shift left for bit definitions in UAPI > headers? Yeah. I knew it was UAPI but I'm not used to thinking about UAPI rules. I only tried to build this on kernel .c files and didn't try to rebuild the usr/ dir. I bet someone would have complained eventually but who would have run into this first... I see there are existing BIT() users in the usr/ dir, but everyone seems good about using __u32 instead of u32. Probably because declaring a variable as u32 causes an immediate compile error for everyone but bogus BIT() defines are not an issue until someone tries to use them. KTODO: write a script to check that UAPI doesn't use kernel types Maybe this could be a part of checkpatch.pl? regards, dan carpenter $ grep BIT usr/ -Rw usr/include/misc/uacce/uacce.h:#define UACCE_DEV_SVA BIT(0) usr/include/linux/psci.h:#define PSCI_1_0_OS_INITIATED BIT(0) usr/include/linux/can/netlink.h: * For further information, please read chapter "8 BIT TIMING usr/include/linux/cxl_mem.h:#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(0) usr/include/linux/cxl_mem.h:#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(1) usr/include/linux/nl80211.h: * bitmask of BIT(NL80211_BAND_*) as described in %enum usr/include/linux/mdio.h:#define MDIO_AN_C73_0_PAUSE BIT(10) usr/include/linux/mdio.h:#define MDIO_AN_C73_0_ASM_DIR BIT(11) usr/include/linux/mdio.h:#define MDIO_AN_C73_0_C2 BIT(12) usr/include/linux/mdio.h:#define MDIO_AN_C73_0_RF BIT(13) usr/include/linux/mdio.h:#define MDIO_AN_C73_0_ACK BIT(14) usr/include/linux/mdio.h:#define MDIO_AN_C73_0_NP BIT(15) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_1000BASE_KX BIT(5) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_10GBASE_KX4 BIT(6) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_10GBASE_KR BIT(7) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_40GBASE_KR4 BIT(8) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_40GBASE_CR4 BIT(9) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_CR10 BIT(10) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_KP4 BIT(11) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_KR4 BIT(12) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_CR4 BIT(13) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_25GBASE_R_S BIT(14) usr/include/linux/mdio.h:#define MDIO_AN_C73_1_25GBASE_R BIT(15) usr/include/linux/mdio.h:#define MDIO_AN_C73_2_2500BASE_KX BIT(0) usr/include/linux/mdio.h:#define MDIO_AN_C73_2_5GBASE_KR BIT(1) usr/include/asm/kvm.h:#define KVM_PMU_EVENT_FLAG_MASKED_EVENTS BIT(0) usr/include/asm/kvm.h:#define KVM_EXIT_HYPERCALL_LONG_MODE BIT(0) usr/include/drm/radeon_drm.h: * THESE ARE NOT BIT FIELDS usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_READ BIT(0) usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_WRITE BIT(1) usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_LBW BIT(2) usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_HBW BIT(3) usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_RR BIT(4) usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_ADDR_DEC BIT(5) $
Hi Dan, On Fri, Nov 03, 2023 at 10:51:09AM +0300, Dan Carpenter wrote: > On Fri, Nov 03, 2023 at 07:32:41AM +0000, Sakari Ailus wrote: > > > > > diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h > > > > > index 4a195b68f28f..21d149969119 100644 > > > > > --- a/include/uapi/linux/v4l2-subdev.h > > > > > +++ b/include/uapi/linux/v4l2-subdev.h > ^^^^ > > > > > > @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { > > > > > * set (which is the default), the 'stream' fields will be forced to 0 by the > > > > > * kernel. > > > > > */ > > > > > - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) > > > > > + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS BIT_ULL(0) > > > > > > > > This is a UAPI header but BIT_ULL() is defined in kernel-only headers. > > > > > > > > So (1ULL << 0) ? > > > > > > > > uapi/linux/const.h also has _BITULL(). > > > > > > Let's just do 1ULL < 0. I'll resend. Is there an automated way I could > > > have caught this? > > > > I don't know. :-) Remember to use shift left for bit definitions in UAPI > > headers? > > Yeah. I knew it was UAPI but I'm not used to thinking about UAPI rules. > I only tried to build this on kernel .c files and didn't try to rebuild > the usr/ dir. > > I bet someone would have complained eventually but who would have > run into this first... I see there are existing BIT() users in the usr/ > dir, but everyone seems good about using __u32 instead of u32. Probably > because declaring a variable as u32 causes an immediate compile error > for everyone but bogus BIT() defines are not an issue until someone > tries to use them. > > KTODO: write a script to check that UAPI doesn't use kernel types > > Maybe this could be a part of checkpatch.pl? I think that would be useful. I wonder if there are be better ways to check for non-existent macros than e.g. singling out BIT() and others. I'd think just checking for BIT() etc. should be enough. > > regards, > dan carpenter > > $ grep BIT usr/ -Rw > usr/include/misc/uacce/uacce.h:#define UACCE_DEV_SVA BIT(0) > usr/include/linux/psci.h:#define PSCI_1_0_OS_INITIATED BIT(0) > usr/include/linux/can/netlink.h: * For further information, please read chapter "8 BIT TIMING > usr/include/linux/cxl_mem.h:#define CXL_MEM_COMMAND_FLAG_ENABLED BIT(0) > usr/include/linux/cxl_mem.h:#define CXL_MEM_COMMAND_FLAG_EXCLUSIVE BIT(1) > usr/include/linux/nl80211.h: * bitmask of BIT(NL80211_BAND_*) as described in %enum > usr/include/linux/mdio.h:#define MDIO_AN_C73_0_PAUSE BIT(10) > usr/include/linux/mdio.h:#define MDIO_AN_C73_0_ASM_DIR BIT(11) > usr/include/linux/mdio.h:#define MDIO_AN_C73_0_C2 BIT(12) > usr/include/linux/mdio.h:#define MDIO_AN_C73_0_RF BIT(13) > usr/include/linux/mdio.h:#define MDIO_AN_C73_0_ACK BIT(14) > usr/include/linux/mdio.h:#define MDIO_AN_C73_0_NP BIT(15) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_1000BASE_KX BIT(5) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_10GBASE_KX4 BIT(6) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_10GBASE_KR BIT(7) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_40GBASE_KR4 BIT(8) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_40GBASE_CR4 BIT(9) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_CR10 BIT(10) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_KP4 BIT(11) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_KR4 BIT(12) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_100GBASE_CR4 BIT(13) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_25GBASE_R_S BIT(14) > usr/include/linux/mdio.h:#define MDIO_AN_C73_1_25GBASE_R BIT(15) > usr/include/linux/mdio.h:#define MDIO_AN_C73_2_2500BASE_KX BIT(0) > usr/include/linux/mdio.h:#define MDIO_AN_C73_2_5GBASE_KR BIT(1) > usr/include/asm/kvm.h:#define KVM_PMU_EVENT_FLAG_MASKED_EVENTS BIT(0) > usr/include/asm/kvm.h:#define KVM_EXIT_HYPERCALL_LONG_MODE BIT(0) > usr/include/drm/radeon_drm.h: * THESE ARE NOT BIT FIELDS > usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_READ BIT(0) > usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_WRITE BIT(1) > usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_LBW BIT(2) > usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_HBW BIT(3) > usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_RR BIT(4) > usr/include/drm/habanalabs_accel.h:#define HL_RAZWI_ADDR_DEC BIT(5) Indeed.
diff --git a/include/uapi/linux/v4l2-subdev.h b/include/uapi/linux/v4l2-subdev.h index 4a195b68f28f..21d149969119 100644 --- a/include/uapi/linux/v4l2-subdev.h +++ b/include/uapi/linux/v4l2-subdev.h @@ -239,7 +239,7 @@ struct v4l2_subdev_routing { * set (which is the default), the 'stream' fields will be forced to 0 by the * kernel. */ - #define V4L2_SUBDEV_CLIENT_CAP_STREAMS (1U << 0) + #define V4L2_SUBDEV_CLIENT_CAP_STREAMS BIT_ULL(0) /** * struct v4l2_subdev_client_capability - Capabilities of the client accessing
The problem is this line here from subdev_do_ioctl(). client_cap->capabilities &= ~V4L2_SUBDEV_CLIENT_CAP_STREAMS; The "client_cap->capabilities" variable is a u64. The AND operation is supposed to clear out the V4L2_SUBDEV_CLIENT_CAP_STREAMS flag. But because it's a 32 bit variable it accidentally clears our the high 32 bits as well. Currently we only use BIT(0) and none ofthe upper bits so this doesn't affect runtime behavior. Fixes: f57fa2959244 ("media: v4l2-subdev: Add new ioctl for client capabilities") Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org> --- include/uapi/linux/v4l2-subdev.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)