[PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Abhishek Pandit-Subedi]

Support setting the uniq attribute of the input device. The uniq
attribute is used as a unique identifier for the connected device.

For example, uinput devices created by BlueZ will store the address of
the connected device as the uniq property.

Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
---
Hi input maintainers,

I added this change to allow BlueZ to display the peer device address in
udev. BlueZ has been setting ATTR{name} to the peer address since it
isn't possible to set the uniq attribute currently.

I've tested this on a Chromebook running kernel v4.19 with this patch.

$ uname -r
4.19.85

$ dmesg | grep "input:" | tail -1
[   69.604752] input: BeatsStudio Wireless as /devices/virtual/input/input17

$ udevadm info -a -p /sys/devices/virtual/input/input17

Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

  looking at device '/devices/virtual/input/input17':
    KERNEL=="input17"
    SUBSYSTEM=="input"
    DRIVER==""
    ATTR{inhibited}=="0"
    ATTR{name}=="BeatsStudio Wireless"
    ATTR{phys}=="00:00:00:6e:d0:74"
    ATTR{properties}=="0"
    ATTR{uniq}=="00:00:00:cc:1c:f3"

(I zeroed out part of the addresses above. The phys attribute
corresponds to the address of the Bluetooth controller on the Chromebook
and the uniq is the address of the headphones)


 drivers/input/misc/uinput.c | 21 ++++++++++++++++++++-
 include/uapi/linux/uinput.h |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 84051f20b18a..68319bda41b8 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -280,7 +280,7 @@ static int uinput_dev_flush(struct input_dev *dev, struct file *file)
 
 static void uinput_destroy_device(struct uinput_device *udev)
 {
-	const char *name, *phys;
+	const char *name, *phys, *uniq;
 	struct input_dev *dev = udev->dev;
 	enum uinput_state old_state = udev->state;
 
@@ -289,6 +289,7 @@ static void uinput_destroy_device(struct uinput_device *udev)
 	if (dev) {
 		name = dev->name;
 		phys = dev->phys;
+		uniq = dev->uniq;
 		if (old_state == UIST_CREATED) {
 			uinput_flush_requests(udev);
 			input_unregister_device(dev);
@@ -297,6 +298,7 @@ static void uinput_destroy_device(struct uinput_device *udev)
 		}
 		kfree(name);
 		kfree(phys);
+		kfree(uniq);
 		udev->dev = NULL;
 	}
 }
@@ -840,6 +842,7 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	struct uinput_ff_erase  ff_erase;
 	struct uinput_request   *req;
 	char			*phys;
+	char			*uniq;
 	const char		*name;
 	unsigned int		size;
 
@@ -931,6 +934,22 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 		udev->dev->phys = phys;
 		goto out;
 
+	case UI_SET_UNIQ:
+		if (udev->state == UIST_CREATED) {
+			retval = -EINVAL;
+			goto out;
+		}
+
+		uniq = strndup_user(p, 1024);
+		if (IS_ERR(uniq)) {
+			retval = PTR_ERR(uniq);
+			goto out;
+		}
+
+		kfree(udev->dev->uniq);
+		udev->dev->uniq = uniq;
+		goto out;
+
 	case UI_BEGIN_FF_UPLOAD:
 		retval = uinput_ff_upload_from_user(p, &ff_up);
 		if (retval)
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index c9e677e3af1d..d5b7767c1b02 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -145,6 +145,7 @@ struct uinput_abs_setup {
 #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
 #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
 #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
+#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
 
 #define UI_BEGIN_FF_UPLOAD	_IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload)
 #define UI_END_FF_UPLOAD	_IOW(UINPUT_IOCTL_BASE, 201, struct uinput_ff_upload)
-- 
2.24.0.432.g9d3f5f5b63-goog

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 1504 bytes --]

Hello!

On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> Support setting the uniq attribute of the input device. The uniq
> attribute is used as a unique identifier for the connected device.
> 
> For example, uinput devices created by BlueZ will store the address of
> the connected device as the uniq property.
> 
> Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>

...

> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c9e677e3af1d..d5b7767c1b02 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -145,6 +145,7 @@ struct uinput_abs_setup {
>  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
>  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
>  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)

I think that usage of char* as type in _IOW would cause compatibility
problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.

I would suggest to define this ioctl as e.g.:

  #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)

And then in uinput.c code handle it as:

  case UI_SET_UNIQ & ~IOCSIZE_MASK:

as part of section /* Now check variable-length commands */

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Dmitry Torokhov]

Hi Pali,

On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> Hello!
> 
> On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > Support setting the uniq attribute of the input device. The uniq
> > attribute is used as a unique identifier for the connected device.
> > 
> > For example, uinput devices created by BlueZ will store the address of
> > the connected device as the uniq property.
> > 
> > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> 
> ...
> 
> > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > index c9e677e3af1d..d5b7767c1b02 100644
> > --- a/include/uapi/linux/uinput.h
> > +++ b/include/uapi/linux/uinput.h
> > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> 
> I think that usage of char* as type in _IOW would cause compatibility
> problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> 
> I would suggest to define this ioctl as e.g.:
> 
>   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> 
> And then in uinput.c code handle it as:
> 
>   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> 
> as part of section /* Now check variable-length commands */

If we did not have UI_SET_PHYS in its current form, I'd agree with you,
but I think there is benefit in having UI_SET_UNIQ be similar to
UI_SET_PHYS.

But you are absolutely correct that in current form the patch is
deficient on 64/32 systems, and the compat handling needs to be added
before it can be accepted.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> Hi Pali,
> 
> On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > Hello!
> > 
> > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > Support setting the uniq attribute of the input device. The uniq
> > > attribute is used as a unique identifier for the connected device.
> > > 
> > > For example, uinput devices created by BlueZ will store the address of
> > > the connected device as the uniq property.
> > > 
> > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > 
> > ...
> > 
> > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > index c9e677e3af1d..d5b7767c1b02 100644
> > > --- a/include/uapi/linux/uinput.h
> > > +++ b/include/uapi/linux/uinput.h
> > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > 
> > I think that usage of char* as type in _IOW would cause compatibility
> > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > 
> > I would suggest to define this ioctl as e.g.:
> > 
> >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > 
> > And then in uinput.c code handle it as:
> > 
> >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > 
> > as part of section /* Now check variable-length commands */
> 
> If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> but I think there is benefit in having UI_SET_UNIQ be similar to
> UI_SET_PHYS.

I thought that ioctl is just number, so we can define it as we want. And
because uinput.c has already switch for variable-length commands it
would be easy to use it. Final handling can be in separate function like
for UI_SET_PHYS which can look like same.

> But you are absolutely correct that in current form the patch is
> deficient on 64/32 systems, and the compat handling needs to be added
> before it can be accepted.

Is not better to avoid usage of compat ioctl? Or it is OK to use compat
ioctl also for new features? I do not know if there are some kernel
rules for it or not... But for me it sounds like "compatibility layer
for older code".

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Dmitry Torokhov]

On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > Hi Pali,
> > 
> > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > Hello!
> > > 
> > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > Support setting the uniq attribute of the input device. The uniq
> > > > attribute is used as a unique identifier for the connected device.
> > > > 
> > > > For example, uinput devices created by BlueZ will store the address of
> > > > the connected device as the uniq property.
> > > > 
> > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > 
> > > ...
> > > 
> > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > --- a/include/uapi/linux/uinput.h
> > > > +++ b/include/uapi/linux/uinput.h
> > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > 
> > > I think that usage of char* as type in _IOW would cause compatibility
> > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > 
> > > I would suggest to define this ioctl as e.g.:
> > > 
> > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > 
> > > And then in uinput.c code handle it as:
> > > 
> > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > 
> > > as part of section /* Now check variable-length commands */
> > 
> > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > but I think there is benefit in having UI_SET_UNIQ be similar to
> > UI_SET_PHYS.
> 
> I thought that ioctl is just number, so we can define it as we want. And
> because uinput.c has already switch for variable-length commands it
> would be easy to use it. Final handling can be in separate function like
> for UI_SET_PHYS which can look like same.

Yes, we can define ioctl number as whatever we want. What I was trying
to say, right now users do this:

	rc = ioctl(fd, UI_SET_PHYS, "whatever");
	...

and with UI_SET_UNIQ they expect the following to work:

	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
	...

They would not expect a variable length IOCTL here, or expect a
fixed-size string, nor do they expect to cast pointer to u64. So keeping
the spirit of UI_SET_PHYS, even if it is not great from 64/32 bit point
of view is beneficial here.

> 
> > But you are absolutely correct that in current form the patch is
> > deficient on 64/32 systems, and the compat handling needs to be added
> > before it can be accepted.
> 
> Is not better to avoid usage of compat ioctl? Or it is OK to use compat
> ioctl also for new features? I do not know if there are some kernel
> rules for it or not... But for me it sounds like "compatibility layer
> for older code".

Yes, if uinput driver did not have any compat code in it, we would not
want to add it. But alas! we already need to handle compat cases for
expsting API, so consistency is more important than purity (in my
opinion) here.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 3981 bytes --]

On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > Hi Pali,
> > > 
> > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > Hello!
> > > > 
> > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > attribute is used as a unique identifier for the connected device.
> > > > > 
> > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > the connected device as the uniq property.
> > > > > 
> > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > 
> > > > ...
> > > > 
> > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > --- a/include/uapi/linux/uinput.h
> > > > > +++ b/include/uapi/linux/uinput.h
> > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > 
> > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > 
> > > > I would suggest to define this ioctl as e.g.:
> > > > 
> > > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > 
> > > > And then in uinput.c code handle it as:
> > > > 
> > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > 
> > > > as part of section /* Now check variable-length commands */
> > > 
> > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > UI_SET_PHYS.
> > 
> > I thought that ioctl is just number, so we can define it as we want. And
> > because uinput.c has already switch for variable-length commands it
> > would be easy to use it. Final handling can be in separate function like
> > for UI_SET_PHYS which can look like same.
> 
> Yes, we can define ioctl number as whatever we want. What I was trying
> to say, right now users do this:
> 
> 	rc = ioctl(fd, UI_SET_PHYS, "whatever");
> 	...
> 
> and with UI_SET_UNIQ they expect the following to work:
> 
> 	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> 	...

And would not following definition

  #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)

allow userspace to call

  rc = ioctl(fd, UI_SET_UNIQ, "whatever");

as you want?

> They would not expect a variable length IOCTL here, or expect a
> fixed-size string, nor do they expect to cast pointer to u64. So keeping
> the spirit of UI_SET_PHYS, even if it is not great from 64/32 bit point
> of view is beneficial here.
> 
> > 
> > > But you are absolutely correct that in current form the patch is
> > > deficient on 64/32 systems, and the compat handling needs to be added
> > > before it can be accepted.
> > 
> > Is not better to avoid usage of compat ioctl? Or it is OK to use compat
> > ioctl also for new features? I do not know if there are some kernel
> > rules for it or not... But for me it sounds like "compatibility layer
> > for older code".
> 
> Yes, if uinput driver did not have any compat code in it, we would not
> want to add it. But alas! we already need to handle compat cases for
> expsting API, so consistency is more important than purity (in my
> opinion) here.
> 
> Thanks.
> 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Dmitry Torokhov]

On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > Hi Pali,
> > > > 
> > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > Hello!
> > > > > 
> > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > 
> > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > the connected device as the uniq property.
> > > > > > 
> > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > 
> > > > > ...
> > > > > 
> > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > 
> > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > 
> > > > > I would suggest to define this ioctl as e.g.:
> > > > > 
> > > > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > 
> > > > > And then in uinput.c code handle it as:
> > > > > 
> > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > 
> > > > > as part of section /* Now check variable-length commands */
> > > > 
> > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > UI_SET_PHYS.
> > > 
> > > I thought that ioctl is just number, so we can define it as we want. And
> > > because uinput.c has already switch for variable-length commands it
> > > would be easy to use it. Final handling can be in separate function like
> > > for UI_SET_PHYS which can look like same.
> > 
> > Yes, we can define ioctl number as whatever we want. What I was trying
> > to say, right now users do this:
> > 
> > 	rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > 	...
> > 
> > and with UI_SET_UNIQ they expect the following to work:
> > 
> > 	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > 	...
> 
> And would not following definition
> 
>   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> 
> allow userspace to call
> 
>   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> 
> as you want?

OK, so what you are saying is that we can have whatever in the size
portion of ioctl number and simply ignore it in the driver (and I do not
think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
While this would work, I am not sure it is the best option as I think
we'd have to comment extensively why we have arbitrary number in place
of the size.

And we still do not really save anything, as we still have to go through
compat ioctl handler (since we have it already) and it is very simple to
add a case for UI_SET_UNIQ there...

Thanks.

-- 
Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Abhishek Pandit-Subedi]

On Mon, Dec 2, 2019 at 11:36 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > Hi Pali,
> > > > >
> > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > Hello!
> > > > > >
> > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > >
> > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > the connected device as the uniq property.
> > > > > > >
> > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > >  #define UI_SET_PHYS                _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > >  #define UI_SET_SWBIT               _IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > >  #define UI_SET_PROPBIT             _IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > +#define UI_SET_UNIQ                _IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > >
> > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > >
> > > > > > I would suggest to define this ioctl as e.g.:
> > > > > >
> > > > > >   #define UI_SET_UNIQ         _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > >
> > > > > > And then in uinput.c code handle it as:
> > > > > >
> > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > >
> > > > > > as part of section /* Now check variable-length commands */
> > > > >
> > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > UI_SET_PHYS.
> > > >
> > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > because uinput.c has already switch for variable-length commands it
> > > > would be easy to use it. Final handling can be in separate function like
> > > > for UI_SET_PHYS which can look like same.
> > >
> > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > to say, right now users do this:
> > >
> > >     rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > >     ...
> > >
> > > and with UI_SET_UNIQ they expect the following to work:
> > >
> > >     rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > >     ...
> >
> > And would not following definition
> >
> >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> >
> > allow userspace to call
> >
> >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> >
> > as you want?
>
> OK, so what you are saying is that we can have whatever in the size
> portion of ioctl number and simply ignore it in the driver (and I do not
> think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> While this would work, I am not sure it is the best option as I think
> we'd have to comment extensively why we have arbitrary number in place
> of the size.
>
> And we still do not really save anything, as we still have to go through
> compat ioctl handler (since we have it already) and it is very simple to
> add a case for UI_SET_UNIQ there...
>
> Thanks.
>
> --
> Dmitry

Since the compat handling already exists for UI_SET_PHYS, I think I
would prefer to go with the simpler solution of just duplicating that
for UI_SET_UNIQ. Next patch is coming with that change.

Thanks
Abhishek

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 5347 bytes --]

On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > Hi Pali,
> > > > > 
> > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > Hello!
> > > > > > 
> > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > 
> > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > the connected device as the uniq property.
> > > > > > > 
> > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > 
> > > > > > ...
> > > > > > 
> > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > 
> > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > 
> > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > 
> > > > > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > 
> > > > > > And then in uinput.c code handle it as:
> > > > > > 
> > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > 
> > > > > > as part of section /* Now check variable-length commands */
> > > > > 
> > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > UI_SET_PHYS.
> > > > 
> > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > because uinput.c has already switch for variable-length commands it
> > > > would be easy to use it. Final handling can be in separate function like
> > > > for UI_SET_PHYS which can look like same.
> > > 
> > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > to say, right now users do this:
> > > 
> > > 	rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > 	...
> > > 
> > > and with UI_SET_UNIQ they expect the following to work:
> > > 
> > > 	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > 	...
> > 
> > And would not following definition
> > 
> >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > 
> > allow userspace to call
> > 
> >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > 
> > as you want?
> 
> OK, so what you are saying is that we can have whatever in the size
> portion of ioctl number and simply ignore it in the driver

Yes.

> (and I do not
> think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).

You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
would be always sam constant number. So it would be really simple. So
original patch would work if UI_SET_UNIQ define would be changed to
above with _IOW() macro.

> While this would work, I am not sure it is the best option as I think
> we'd have to comment extensively why we have arbitrary number in place
> of the size.

Comment can be added. But this is as ioctl is going to accept variable
length array (not fixed array), zero value make sense for me (zero as we
do not know exact size).

> And we still do not really save anything, as we still have to go through
> compat ioctl handler (since we have it already) and it is very simple to
> add a case for UI_SET_UNIQ there...

Yes, compat ioctl is still used. But my proposed solution does not
involve to define a new compact ioctl number just for sizeof(char *).

I'm looking at this particular problem from side, that there is no
reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
number and one compat number), when one number is enough. It is one new
ioctl call, so one ioctl number should be enough.

And also with my proposed solution with ioctl size=0 it simplify
implementation of UI_SET_UNIQ as it is not needed to implement also
UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
original patch (with changed UI_SET_UNIQ macro) is enough.

But of of course, this is my view of this problem and I would not be
against your decision from maintainer position. Both solutions would
work correctly and bring same behavior for userspace applications.

> Thanks.
> 

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 6878 bytes --]

On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > Hi Pali,
> > > > > > 
> > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > Hello!
> > > > > > > 
> > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > 
> > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > the connected device as the uniq property.
> > > > > > > > 
> > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > 
> > > > > > > ...
> > > > > > > 
> > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > 
> > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > 
> > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > 
> > > > > > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > 
> > > > > > > And then in uinput.c code handle it as:
> > > > > > > 
> > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > 
> > > > > > > as part of section /* Now check variable-length commands */
> > > > > > 
> > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > UI_SET_PHYS.
> > > > > 
> > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > because uinput.c has already switch for variable-length commands it
> > > > > would be easy to use it. Final handling can be in separate function like
> > > > > for UI_SET_PHYS which can look like same.
> > > > 
> > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > to say, right now users do this:
> > > > 
> > > > 	rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > 	...
> > > > 
> > > > and with UI_SET_UNIQ they expect the following to work:
> > > > 
> > > > 	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > 	...
> > > 
> > > And would not following definition
> > > 
> > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > 
> > > allow userspace to call
> > > 
> > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > 
> > > as you want?
> > 
> > OK, so what you are saying is that we can have whatever in the size
> > portion of ioctl number and simply ignore it in the driver
> 
> Yes.
> 
> > (and I do not
> > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> 
> You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> would be always sam constant number. So it would be really simple. So
> original patch would work if UI_SET_UNIQ define would be changed to
> above with _IOW() macro.
> 
> > While this would work, I am not sure it is the best option as I think
> > we'd have to comment extensively why we have arbitrary number in place
> > of the size.
> 
> Comment can be added. But this is as ioctl is going to accept variable
> length array (not fixed array), zero value make sense for me (zero as we
> do not know exact size).
> 
> > And we still do not really save anything, as we still have to go through
> > compat ioctl handler (since we have it already) and it is very simple to
> > add a case for UI_SET_UNIQ there...
> 
> Yes, compat ioctl is still used. But my proposed solution does not
> involve to define a new compact ioctl number just for sizeof(char *).
> 
> I'm looking at this particular problem from side, that there is no
> reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> number and one compat number), when one number is enough. It is one new
> ioctl call, so one ioctl number should be enough.
> 
> And also with my proposed solution with ioctl size=0 it simplify
> implementation of UI_SET_UNIQ as it is not needed to implement also
> UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> original patch (with changed UI_SET_UNIQ macro) is enough.
> 
> But of of course, this is my view of this problem and I would not be
> against your decision from maintainer position. Both solutions would
> work correctly and bring same behavior for userspace applications.


Hi Dmitry!

I was looking again at those _IOW defines for ioctl calls and I have
another argument why not specify 'char *' in _IOW:

All ioctls in _IOW() specify as a third macro argument type which is
passed as pointer to the third argument for ioctl() syscall.

So e.g.:

  #define EVIOCSCLOCKID _IOW('E', 0xa0, int)

is called from userspace as:

  int val;
  ioctl(fd, EVIOCSCLOCKID, &val);

Or

  #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)

is called as:

  struct input_mask val;
  ioctl(fd, EVIOCSMASK, &val);

So basically third argument for _IOW specify size of byte buffer passed
as third argument for ioctl(). In _IOW is not specified pointer to
struct input_mask, but struct input_mask itself.

And in case you define

  #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)

then you by above usage you should pass data as:

  char *val = "DATA";
  ioctl(fd, MY_NEW_IOCTL, &val);

Which is not same as just:

  ioctl(fd, MY_NEW_IOCTL, "DATA");

As in former case you passed pointer to pointer to data and in later
case you passed only pointer to data.

It just mean that UI_SET_PHYS is already defined inconsistently which is
also reason why compat ioctl for it was introduced.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Dmitry Torokhov]

On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > > Hi Pali,
> > > > > > > 
> > > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > > Hello!
> > > > > > > > 
> > > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > > 
> > > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > > the connected device as the uniq property.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > > 
> > > > > > > > ...
> > > > > > > > 
> > > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > > 
> > > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > > 
> > > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > > 
> > > > > > > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > > 
> > > > > > > > And then in uinput.c code handle it as:
> > > > > > > > 
> > > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > > 
> > > > > > > > as part of section /* Now check variable-length commands */
> > > > > > > 
> > > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > > UI_SET_PHYS.
> > > > > > 
> > > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > > because uinput.c has already switch for variable-length commands it
> > > > > > would be easy to use it. Final handling can be in separate function like
> > > > > > for UI_SET_PHYS which can look like same.
> > > > > 
> > > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > > to say, right now users do this:
> > > > > 
> > > > > 	rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > > 	...
> > > > > 
> > > > > and with UI_SET_UNIQ they expect the following to work:
> > > > > 
> > > > > 	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > 	...
> > > > 
> > > > And would not following definition
> > > > 
> > > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > 
> > > > allow userspace to call
> > > > 
> > > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > 
> > > > as you want?
> > > 
> > > OK, so what you are saying is that we can have whatever in the size
> > > portion of ioctl number and simply ignore it in the driver
> > 
> > Yes.
> > 
> > > (and I do not
> > > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> > 
> > You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> > would be always sam constant number. So it would be really simple. So
> > original patch would work if UI_SET_UNIQ define would be changed to
> > above with _IOW() macro.
> > 
> > > While this would work, I am not sure it is the best option as I think
> > > we'd have to comment extensively why we have arbitrary number in place
> > > of the size.
> > 
> > Comment can be added. But this is as ioctl is going to accept variable
> > length array (not fixed array), zero value make sense for me (zero as we
> > do not know exact size).
> > 
> > > And we still do not really save anything, as we still have to go through
> > > compat ioctl handler (since we have it already) and it is very simple to
> > > add a case for UI_SET_UNIQ there...
> > 
> > Yes, compat ioctl is still used. But my proposed solution does not
> > involve to define a new compact ioctl number just for sizeof(char *).
> > 
> > I'm looking at this particular problem from side, that there is no
> > reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> > number and one compat number), when one number is enough. It is one new
> > ioctl call, so one ioctl number should be enough.
> > 
> > And also with my proposed solution with ioctl size=0 it simplify
> > implementation of UI_SET_UNIQ as it is not needed to implement also
> > UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> > original patch (with changed UI_SET_UNIQ macro) is enough.
> > 
> > But of of course, this is my view of this problem and I would not be
> > against your decision from maintainer position. Both solutions would
> > work correctly and bring same behavior for userspace applications.
> 
> 
> Hi Dmitry!
> 
> I was looking again at those _IOW defines for ioctl calls and I have
> another argument why not specify 'char *' in _IOW:
> 
> All ioctls in _IOW() specify as a third macro argument type which is
> passed as pointer to the third argument for ioctl() syscall.
> 
> So e.g.:
> 
>   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> 
> is called from userspace as:
> 
>   int val;
>   ioctl(fd, EVIOCSCLOCKID, &val);
> 
> Or
> 
>   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> 
> is called as:
> 
>   struct input_mask val;
>   ioctl(fd, EVIOCSMASK, &val);
> 
> So basically third argument for _IOW specify size of byte buffer passed
> as third argument for ioctl(). In _IOW is not specified pointer to
> struct input_mask, but struct input_mask itself.
> 
> And in case you define
> 
>   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> 
> then you by above usage you should pass data as:
> 
>   char *val = "DATA";
>   ioctl(fd, MY_NEW_IOCTL, &val);
> 
> Which is not same as just:
> 
>   ioctl(fd, MY_NEW_IOCTL, "DATA");
> 
> As in former case you passed pointer to pointer to data and in later
> case you passed only pointer to data.
> 
> It just mean that UI_SET_PHYS is already defined inconsistently which is
> also reason why compat ioctl for it was introduced.

Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
what to do with all of this...

Maybe we should define

#define UI_SET_PHYS_STR(len)	_IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
#define UI_SET_UNIQ_STR(len)	_IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)

and mark UI_SET_PHYS as deprecated/wrong? This will allow us to specify
exactly how much data kernel is supposed to fetch from userspace instead
of trying to rely on a null-terminated string.

It would also be very helpful if BlueZ did not accept changes that use
this brand new ioctl until after we agreed on how it should look like.
Luiz, can it be reverted for now please?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Marcel Holtmann]

Hi Abhishek,

> Support setting the uniq attribute of the input device. The uniq
> attribute is used as a unique identifier for the connected device.
> 
> For example, uinput devices created by BlueZ will store the address of
> the connected device as the uniq property.

you might also then want to add HIDIOCGRAWUNIQ support to the hidraw driver.

Regards

Marcel

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Luiz Augusto von Dentz]

Hi Dmitry,

On Tue, Dec 3, 2019 at 9:11 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > > > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > > > Hello!
> > > > > > > > >
> > > > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > > >
> > > > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > > > the connected device as the uniq property.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > > > >  #define UI_SET_PHYS          _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > > > >  #define UI_SET_SWBIT         _IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > > > >  #define UI_SET_PROPBIT               _IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > > > +#define UI_SET_UNIQ          _IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > > >
> > > > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > > >
> > > > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > > >
> > > > > > > > >   #define UI_SET_UNIQ           _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > > >
> > > > > > > > > And then in uinput.c code handle it as:
> > > > > > > > >
> > > > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > > >
> > > > > > > > > as part of section /* Now check variable-length commands */
> > > > > > > >
> > > > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > > > UI_SET_PHYS.
> > > > > > >
> > > > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > > > because uinput.c has already switch for variable-length commands it
> > > > > > > would be easy to use it. Final handling can be in separate function like
> > > > > > > for UI_SET_PHYS which can look like same.
> > > > > >
> > > > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > > > to say, right now users do this:
> > > > > >
> > > > > >       rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > > >       ...
> > > > > >
> > > > > > and with UI_SET_UNIQ they expect the following to work:
> > > > > >
> > > > > >       rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > >       ...
> > > > >
> > > > > And would not following definition
> > > > >
> > > > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > >
> > > > > allow userspace to call
> > > > >
> > > > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > >
> > > > > as you want?
> > > >
> > > > OK, so what you are saying is that we can have whatever in the size
> > > > portion of ioctl number and simply ignore it in the driver
> > >
> > > Yes.
> > >
> > > > (and I do not
> > > > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> > >
> > > You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> > > would be always sam constant number. So it would be really simple. So
> > > original patch would work if UI_SET_UNIQ define would be changed to
> > > above with _IOW() macro.
> > >
> > > > While this would work, I am not sure it is the best option as I think
> > > > we'd have to comment extensively why we have arbitrary number in place
> > > > of the size.
> > >
> > > Comment can be added. But this is as ioctl is going to accept variable
> > > length array (not fixed array), zero value make sense for me (zero as we
> > > do not know exact size).
> > >
> > > > And we still do not really save anything, as we still have to go through
> > > > compat ioctl handler (since we have it already) and it is very simple to
> > > > add a case for UI_SET_UNIQ there...
> > >
> > > Yes, compat ioctl is still used. But my proposed solution does not
> > > involve to define a new compact ioctl number just for sizeof(char *).
> > >
> > > I'm looking at this particular problem from side, that there is no
> > > reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> > > number and one compat number), when one number is enough. It is one new
> > > ioctl call, so one ioctl number should be enough.
> > >
> > > And also with my proposed solution with ioctl size=0 it simplify
> > > implementation of UI_SET_UNIQ as it is not needed to implement also
> > > UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> > > original patch (with changed UI_SET_UNIQ macro) is enough.
> > >
> > > But of of course, this is my view of this problem and I would not be
> > > against your decision from maintainer position. Both solutions would
> > > work correctly and bring same behavior for userspace applications.
> >
> >
> > Hi Dmitry!
> >
> > I was looking again at those _IOW defines for ioctl calls and I have
> > another argument why not specify 'char *' in _IOW:
> >
> > All ioctls in _IOW() specify as a third macro argument type which is
> > passed as pointer to the third argument for ioctl() syscall.
> >
> > So e.g.:
> >
> >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> >
> > is called from userspace as:
> >
> >   int val;
> >   ioctl(fd, EVIOCSCLOCKID, &val);
> >
> > Or
> >
> >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> >
> > is called as:
> >
> >   struct input_mask val;
> >   ioctl(fd, EVIOCSMASK, &val);
> >
> > So basically third argument for _IOW specify size of byte buffer passed
> > as third argument for ioctl(). In _IOW is not specified pointer to
> > struct input_mask, but struct input_mask itself.
> >
> > And in case you define
> >
> >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> >
> > then you by above usage you should pass data as:
> >
> >   char *val = "DATA";
> >   ioctl(fd, MY_NEW_IOCTL, &val);
> >
> > Which is not same as just:
> >
> >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> >
> > As in former case you passed pointer to pointer to data and in later
> > case you passed only pointer to data.
> >
> > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > also reason why compat ioctl for it was introduced.
>
> Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> what to do with all of this...
>
> Maybe we should define
>
> #define UI_SET_PHYS_STR(len)    _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> #define UI_SET_UNIQ_STR(len)    _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
>
> and mark UI_SET_PHYS as deprecated/wrong? This will allow us to specify
> exactly how much data kernel is supposed to fetch from userspace instead
> of trying to rely on a null-terminated string.
>
> It would also be very helpful if BlueZ did not accept changes that use
> this brand new ioctl until after we agreed on how it should look like.
> Luiz, can it be reverted for now please?

Sure, it has been reverted, I guess we will have to settle on the
kernel changes first before we do any changes to userspace.

-- 
Luiz Augusto von Dentz

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Abhishek Pandit-Subedi]

Hi Dmitry and Pali,

I refactored the ioctl handlers as described above and tested it. It
seems to be working without any compat changes.

I compiled the following code in both 32-bit (gcc -m32 test.c) and
64-bit to test.

Please take a look at the new patch.

Thanks
Abhishek

test.c
---
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <string.h>
#include <sys/ioctl.h>
#include <unistd.h>

#include "uinput.h"

int foo(int fd) {
        struct uinput_dev dev;
        int ret;

        memset(&dev, 0, sizeof(dev));

        dev.id.bustype = BUS_BLUETOOTH;
        dev.id.vendor = 0x3;
        dev.id.product = 0x4;
        dev.id.version = 0x5;

        memcpy(dev.name, "Test", 4);

        printf("Setting bus/vendor/product/version\n");
        if (write(fd, &dev, sizeof(dev)) < 0) {
                perror("write");
                return errno;
        }

        printf("Making ioctl calls\n");
        ioctl(fd, UI_SET_EVBIT, EV_KEY);
        ioctl(fd, UI_SET_EVBIT, EV_REL);
        ioctl(fd, UI_SET_EVBIT, EV_REP);
        ioctl(fd, UI_SET_EVBIT, EV_SYN);

        /* I also replaced this with UI_SET_PHYS to check for the
deprecation notice. */
        if (ioctl(fd, UI_SET_PHYS_STR(18), "00:00:00:33:44:55") < 0) {
                perror("ioctl UI_SET_PHYS");
                return errno;
        }

        if (ioctl(fd, UI_SET_UNIQ_STR(18), "00:11:22:00:00:00") < 0) {
                perror("ioctl UI_SET_UNIQ");
                return errno;
        }

        if (ioctl(fd, UI_DEV_CREATE, NULL) < 0) {
                perror("ioctl UI_DEV_CREATE");
                return errno;
        }

        return 0;
}

int main() {
        int fd, ret;

        fd = open("/dev/uinput", O_RDWR);

        if (fd < 0) {
                perror("open");
                return fd;
        }

        printf("Opened fd %d for write\n", fd);
        ret = foo(fd);

        if (!ret) {
                printf("Uinput has been prepared. Check the uniq value.\n");
                printf("Sleeping for 15s...\n");
                sleep(20);
        }

        close(fd);
        return ret;
}

On Tue, Dec 3, 2019 at 11:11 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > > > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > > > Hi Pali,
> > > > > > > >
> > > > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > > > Hello!
> > > > > > > > >
> > > > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > > >
> > > > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > > > the connected device as the uniq property.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > > >
> > > > > > > > > ...
> > > > > > > > >
> > > > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > > > >  #define UI_SET_PHYS          _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > > > >  #define UI_SET_SWBIT         _IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > > > >  #define UI_SET_PROPBIT               _IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > > > +#define UI_SET_UNIQ          _IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > > >
> > > > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > > >
> > > > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > > >
> > > > > > > > >   #define UI_SET_UNIQ           _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > > >
> > > > > > > > > And then in uinput.c code handle it as:
> > > > > > > > >
> > > > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > > >
> > > > > > > > > as part of section /* Now check variable-length commands */
> > > > > > > >
> > > > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > > > UI_SET_PHYS.
> > > > > > >
> > > > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > > > because uinput.c has already switch for variable-length commands it
> > > > > > > would be easy to use it. Final handling can be in separate function like
> > > > > > > for UI_SET_PHYS which can look like same.
> > > > > >
> > > > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > > > to say, right now users do this:
> > > > > >
> > > > > >       rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > > >       ...
> > > > > >
> > > > > > and with UI_SET_UNIQ they expect the following to work:
> > > > > >
> > > > > >       rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > >       ...
> > > > >
> > > > > And would not following definition
> > > > >
> > > > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > >
> > > > > allow userspace to call
> > > > >
> > > > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > >
> > > > > as you want?
> > > >
> > > > OK, so what you are saying is that we can have whatever in the size
> > > > portion of ioctl number and simply ignore it in the driver
> > >
> > > Yes.
> > >
> > > > (and I do not
> > > > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> > >
> > > You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> > > would be always sam constant number. So it would be really simple. So
> > > original patch would work if UI_SET_UNIQ define would be changed to
> > > above with _IOW() macro.
> > >
> > > > While this would work, I am not sure it is the best option as I think
> > > > we'd have to comment extensively why we have arbitrary number in place
> > > > of the size.
> > >
> > > Comment can be added. But this is as ioctl is going to accept variable
> > > length array (not fixed array), zero value make sense for me (zero as we
> > > do not know exact size).
> > >
> > > > And we still do not really save anything, as we still have to go through
> > > > compat ioctl handler (since we have it already) and it is very simple to
> > > > add a case for UI_SET_UNIQ there...
> > >
> > > Yes, compat ioctl is still used. But my proposed solution does not
> > > involve to define a new compact ioctl number just for sizeof(char *).
> > >
> > > I'm looking at this particular problem from side, that there is no
> > > reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> > > number and one compat number), when one number is enough. It is one new
> > > ioctl call, so one ioctl number should be enough.
> > >
> > > And also with my proposed solution with ioctl size=0 it simplify
> > > implementation of UI_SET_UNIQ as it is not needed to implement also
> > > UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> > > original patch (with changed UI_SET_UNIQ macro) is enough.
> > >
> > > But of of course, this is my view of this problem and I would not be
> > > against your decision from maintainer position. Both solutions would
> > > work correctly and bring same behavior for userspace applications.
> >
> >
> > Hi Dmitry!
> >
> > I was looking again at those _IOW defines for ioctl calls and I have
> > another argument why not specify 'char *' in _IOW:
> >
> > All ioctls in _IOW() specify as a third macro argument type which is
> > passed as pointer to the third argument for ioctl() syscall.
> >
> > So e.g.:
> >
> >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> >
> > is called from userspace as:
> >
> >   int val;
> >   ioctl(fd, EVIOCSCLOCKID, &val);
> >
> > Or
> >
> >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> >
> > is called as:
> >
> >   struct input_mask val;
> >   ioctl(fd, EVIOCSMASK, &val);
> >
> > So basically third argument for _IOW specify size of byte buffer passed
> > as third argument for ioctl(). In _IOW is not specified pointer to
> > struct input_mask, but struct input_mask itself.
> >
> > And in case you define
> >
> >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> >
> > then you by above usage you should pass data as:
> >
> >   char *val = "DATA";
> >   ioctl(fd, MY_NEW_IOCTL, &val);
> >
> > Which is not same as just:
> >
> >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> >
> > As in former case you passed pointer to pointer to data and in later
> > case you passed only pointer to data.
> >
> > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > also reason why compat ioctl for it was introduced.
>
> Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> what to do with all of this...
>
> Maybe we should define
>
> #define UI_SET_PHYS_STR(len)    _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> #define UI_SET_UNIQ_STR(len)    _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
>
> and mark UI_SET_PHYS as deprecated/wrong? This will allow us to specify
> exactly how much data kernel is supposed to fetch from userspace instead
> of trying to rely on a null-terminated string.
>
> It would also be very helpful if BlueZ did not accept changes that use
> this brand new ioctl until after we agreed on how it should look like.
> Luiz, can it be reverted for now please?
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > > > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > > > Hi Pali,
> > > > > > > > 
> > > > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > > > Hello!
> > > > > > > > > 
> > > > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > > > 
> > > > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > > > the connected device as the uniq property.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > > > 
> > > > > > > > > ...
> > > > > > > > > 
> > > > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > > > >  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > > > >  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > > > >  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > > > +#define UI_SET_UNIQ		_IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > > > 
> > > > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > > > 
> > > > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > > > 
> > > > > > > > >   #define UI_SET_UNIQ		_IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > > > 
> > > > > > > > > And then in uinput.c code handle it as:
> > > > > > > > > 
> > > > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > > > 
> > > > > > > > > as part of section /* Now check variable-length commands */
> > > > > > > > 
> > > > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > > > UI_SET_PHYS.
> > > > > > > 
> > > > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > > > because uinput.c has already switch for variable-length commands it
> > > > > > > would be easy to use it. Final handling can be in separate function like
> > > > > > > for UI_SET_PHYS which can look like same.
> > > > > > 
> > > > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > > > to say, right now users do this:
> > > > > > 
> > > > > > 	rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > > > 	...
> > > > > > 
> > > > > > and with UI_SET_UNIQ they expect the following to work:
> > > > > > 
> > > > > > 	rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > > 	...
> > > > > 
> > > > > And would not following definition
> > > > > 
> > > > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > 
> > > > > allow userspace to call
> > > > > 
> > > > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > 
> > > > > as you want?
> > > > 
> > > > OK, so what you are saying is that we can have whatever in the size
> > > > portion of ioctl number and simply ignore it in the driver
> > > 
> > > Yes.
> > > 
> > > > (and I do not
> > > > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> > > 
> > > You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> > > would be always sam constant number. So it would be really simple. So
> > > original patch would work if UI_SET_UNIQ define would be changed to
> > > above with _IOW() macro.
> > > 
> > > > While this would work, I am not sure it is the best option as I think
> > > > we'd have to comment extensively why we have arbitrary number in place
> > > > of the size.
> > > 
> > > Comment can be added. But this is as ioctl is going to accept variable
> > > length array (not fixed array), zero value make sense for me (zero as we
> > > do not know exact size).
> > > 
> > > > And we still do not really save anything, as we still have to go through
> > > > compat ioctl handler (since we have it already) and it is very simple to
> > > > add a case for UI_SET_UNIQ there...
> > > 
> > > Yes, compat ioctl is still used. But my proposed solution does not
> > > involve to define a new compact ioctl number just for sizeof(char *).
> > > 
> > > I'm looking at this particular problem from side, that there is no
> > > reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> > > number and one compat number), when one number is enough. It is one new
> > > ioctl call, so one ioctl number should be enough.
> > > 
> > > And also with my proposed solution with ioctl size=0 it simplify
> > > implementation of UI_SET_UNIQ as it is not needed to implement also
> > > UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> > > original patch (with changed UI_SET_UNIQ macro) is enough.
> > > 
> > > But of of course, this is my view of this problem and I would not be
> > > against your decision from maintainer position. Both solutions would
> > > work correctly and bring same behavior for userspace applications.
> > 
> > 
> > Hi Dmitry!
> > 
> > I was looking again at those _IOW defines for ioctl calls and I have
> > another argument why not specify 'char *' in _IOW:
> > 
> > All ioctls in _IOW() specify as a third macro argument type which is
> > passed as pointer to the third argument for ioctl() syscall.
> > 
> > So e.g.:
> > 
> >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > 
> > is called from userspace as:
> > 
> >   int val;
> >   ioctl(fd, EVIOCSCLOCKID, &val);
> > 
> > Or
> > 
> >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > 
> > is called as:
> > 
> >   struct input_mask val;
> >   ioctl(fd, EVIOCSMASK, &val);
> > 
> > So basically third argument for _IOW specify size of byte buffer passed
> > as third argument for ioctl(). In _IOW is not specified pointer to
> > struct input_mask, but struct input_mask itself.
> > 
> > And in case you define
> > 
> >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > 
> > then you by above usage you should pass data as:
> > 
> >   char *val = "DATA";
> >   ioctl(fd, MY_NEW_IOCTL, &val);
> > 
> > Which is not same as just:
> > 
> >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > 
> > As in former case you passed pointer to pointer to data and in later
> > case you passed only pointer to data.
> > 
> > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > also reason why compat ioctl for it was introduced.
> 
> Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> what to do with all of this...
> 
> Maybe we should define
> 
> #define UI_SET_PHYS_STR(len)	_IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> #define UI_SET_UNIQ_STR(len)	_IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)

I'm not sure if this is ideal. Normally in C strings are nul-termined,
so functions/macros do not take buffer length. _STR therefore in names
looks inconsistent.

Maybe this is something which should be handled and unified at global
scope of linux include files. Or al least make consensus how should be
new ioctls for linux written.

Otherwise each API would use different ioctl schema and mess would be
still there. Which means that defining a new ioctl UI_SET_PHYS_STR for
existing one UI_SET_PHYS does not fix anything -- but rather make mess a
big larger.

Or is there already some discussion on LKML about this ioctl problem?

> and mark UI_SET_PHYS as deprecated/wrong? This will allow us to specify
> exactly how much data kernel is supposed to fetch from userspace instead
> of trying to rely on a null-terminated string.
> 
> It would also be very helpful if BlueZ did not accept changes that use
> this brand new ioctl until after we agreed on how it should look like.
> Luiz, can it be reverted for now please?
> 
> Thanks.
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Abhishek Pandit-Subedi]

On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > > > > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > > > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > > > > Hi Pali,
> > > > > > > > >
> > > > > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > > > > Hello!
> > > > > > > > > >
> > > > > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > > > >
> > > > > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > > > > the connected device as the uniq property.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > > > >
> > > > > > > > > > ...
> > > > > > > > > >
> > > > > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > > > > >  #define UI_SET_PHYS                _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > > > > >  #define UI_SET_SWBIT               _IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > > > > >  #define UI_SET_PROPBIT             _IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > > > > +#define UI_SET_UNIQ                _IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > > > >
> > > > > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > > > >
> > > > > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > > > >
> > > > > > > > > >   #define UI_SET_UNIQ         _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > > > >
> > > > > > > > > > And then in uinput.c code handle it as:
> > > > > > > > > >
> > > > > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > > > >
> > > > > > > > > > as part of section /* Now check variable-length commands */
> > > > > > > > >
> > > > > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > > > > UI_SET_PHYS.
> > > > > > > >
> > > > > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > > > > because uinput.c has already switch for variable-length commands it
> > > > > > > > would be easy to use it. Final handling can be in separate function like
> > > > > > > > for UI_SET_PHYS which can look like same.
> > > > > > >
> > > > > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > > > > to say, right now users do this:
> > > > > > >
> > > > > > >     rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > > > >     ...
> > > > > > >
> > > > > > > and with UI_SET_UNIQ they expect the following to work:
> > > > > > >
> > > > > > >     rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > > >     ...
> > > > > >
> > > > > > And would not following definition
> > > > > >
> > > > > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > >
> > > > > > allow userspace to call
> > > > > >
> > > > > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > >
> > > > > > as you want?
> > > > >
> > > > > OK, so what you are saying is that we can have whatever in the size
> > > > > portion of ioctl number and simply ignore it in the driver
> > > >
> > > > Yes.
> > > >
> > > > > (and I do not
> > > > > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> > > >
> > > > You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> > > > would be always sam constant number. So it would be really simple. So
> > > > original patch would work if UI_SET_UNIQ define would be changed to
> > > > above with _IOW() macro.
> > > >
> > > > > While this would work, I am not sure it is the best option as I think
> > > > > we'd have to comment extensively why we have arbitrary number in place
> > > > > of the size.
> > > >
> > > > Comment can be added. But this is as ioctl is going to accept variable
> > > > length array (not fixed array), zero value make sense for me (zero as we
> > > > do not know exact size).
> > > >
> > > > > And we still do not really save anything, as we still have to go through
> > > > > compat ioctl handler (since we have it already) and it is very simple to
> > > > > add a case for UI_SET_UNIQ there...
> > > >
> > > > Yes, compat ioctl is still used. But my proposed solution does not
> > > > involve to define a new compact ioctl number just for sizeof(char *).
> > > >
> > > > I'm looking at this particular problem from side, that there is no
> > > > reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> > > > number and one compat number), when one number is enough. It is one new
> > > > ioctl call, so one ioctl number should be enough.
> > > >
> > > > And also with my proposed solution with ioctl size=0 it simplify
> > > > implementation of UI_SET_UNIQ as it is not needed to implement also
> > > > UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> > > > original patch (with changed UI_SET_UNIQ macro) is enough.
> > > >
> > > > But of of course, this is my view of this problem and I would not be
> > > > against your decision from maintainer position. Both solutions would
> > > > work correctly and bring same behavior for userspace applications.
> > >
> > >
> > > Hi Dmitry!
> > >
> > > I was looking again at those _IOW defines for ioctl calls and I have
> > > another argument why not specify 'char *' in _IOW:
> > >
> > > All ioctls in _IOW() specify as a third macro argument type which is
> > > passed as pointer to the third argument for ioctl() syscall.
> > >
> > > So e.g.:
> > >
> > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > >
> > > is called from userspace as:
> > >
> > >   int val;
> > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > >
> > > Or
> > >
> > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > >
> > > is called as:
> > >
> > >   struct input_mask val;
> > >   ioctl(fd, EVIOCSMASK, &val);
> > >
> > > So basically third argument for _IOW specify size of byte buffer passed
> > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > struct input_mask, but struct input_mask itself.
> > >
> > > And in case you define
> > >
> > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > >
> > > then you by above usage you should pass data as:
> > >
> > >   char *val = "DATA";
> > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > >
> > > Which is not same as just:
> > >
> > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > >
> > > As in former case you passed pointer to pointer to data and in later
> > > case you passed only pointer to data.
> > >
> > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > also reason why compat ioctl for it was introduced.
> >
> > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > what to do with all of this...
> >
> > Maybe we should define
> >
> > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
>
> I'm not sure if this is ideal. Normally in C strings are nul-termined,
> so functions/macros do not take buffer length.
Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
the user to kernel boundary of ioctl, I think we should require size
of the user buffer regardless of the data type.

> _STR therefore in names looks inconsistent.
The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
requiring the length seems to be common across various ioctls.
* input.h requires a length when requesting the phys and uniq
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
* Same with HIDRAW when setting and getting features:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88

>
> Maybe this is something which should be handled and unified at global
> scope of linux include files. Or al least make consensus how should be
> new ioctls for linux written.
>
> Otherwise each API would use different ioctl schema and mess would be
> still there. Which means that defining a new ioctl UI_SET_PHYS_STR for
> existing one UI_SET_PHYS does not fix anything -- but rather make mess a
> big larger.
>
> Or is there already some discussion on LKML about this ioctl problem?
I found this fairly old email (couldn't find something more recent):
http://lkml.iu.edu/hypermail/linux/kernel/9903.3/0325.html
I think the intent is for userspace to provide the size of the string
they're passing in (or at least the size of the allocated buffer that
has the string).

>
> > and mark UI_SET_PHYS as deprecated/wrong? This will allow us to specify
> > exactly how much data kernel is supposed to fetch from userspace instead
> > of trying to rely on a null-terminated string.
> >
> > It would also be very helpful if BlueZ did not accept changes that use
> > this brand new ioctl until after we agreed on how it should look like.
> > Luiz, can it be reverted for now please?
> >
> > Thanks.
> >
>
> --
> Pali Rohár
> pali.rohar@gmail.com


Abhishek

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > On Monday 02 December 2019 11:36:28 Dmitry Torokhov wrote:
> > > > > > On Mon, Dec 02, 2019 at 07:53:40PM +0100, Pali Rohár wrote:
> > > > > > > On Monday 02 December 2019 09:54:40 Dmitry Torokhov wrote:
> > > > > > > > On Mon, Dec 02, 2019 at 09:47:50AM +0100, Pali Rohár wrote:
> > > > > > > > > On Sunday 01 December 2019 17:23:05 Dmitry Torokhov wrote:
> > > > > > > > > > Hi Pali,
> > > > > > > > > >
> > > > > > > > > > On Sun, Dec 01, 2019 at 03:53:57PM +0100, Pali Rohár wrote:
> > > > > > > > > > > Hello!
> > > > > > > > > > >
> > > > > > > > > > > On Wednesday 27 November 2019 10:51:39 Abhishek Pandit-Subedi wrote:
> > > > > > > > > > > > Support setting the uniq attribute of the input device. The uniq
> > > > > > > > > > > > attribute is used as a unique identifier for the connected device.
> > > > > > > > > > > >
> > > > > > > > > > > > For example, uinput devices created by BlueZ will store the address of
> > > > > > > > > > > > the connected device as the uniq property.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
> > > > > > > > > > >
> > > > > > > > > > > ...
> > > > > > > > > > >
> > > > > > > > > > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > > > > > > > > > index c9e677e3af1d..d5b7767c1b02 100644
> > > > > > > > > > > > --- a/include/uapi/linux/uinput.h
> > > > > > > > > > > > +++ b/include/uapi/linux/uinput.h
> > > > > > > > > > > > @@ -145,6 +145,7 @@ struct uinput_abs_setup {
> > > > > > > > > > > >  #define UI_SET_PHYS                _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > > > > > > > > > >  #define UI_SET_SWBIT               _IOW(UINPUT_IOCTL_BASE, 109, int)
> > > > > > > > > > > >  #define UI_SET_PROPBIT             _IOW(UINPUT_IOCTL_BASE, 110, int)
> > > > > > > > > > > > +#define UI_SET_UNIQ                _IOW(UINPUT_IOCTL_BASE, 111, char*)
> > > > > > > > > > >
> > > > > > > > > > > I think that usage of char* as type in _IOW would cause compatibility
> > > > > > > > > > > problems like it is for UI_SET_PHYS (there is UI_SET_PHYS_COMPAT). Size
> > > > > > > > > > > of char* pointer depends on userspace (32 vs 64bit), so 32bit process on
> > > > > > > > > > > 64bit kernel would not be able to call this new UI_SET_UNIQ ioctl.
> > > > > > > > > > >
> > > > > > > > > > > I would suggest to define this ioctl as e.g.:
> > > > > > > > > > >
> > > > > > > > > > >   #define UI_SET_UNIQ         _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > > > > > >
> > > > > > > > > > > And then in uinput.c code handle it as:
> > > > > > > > > > >
> > > > > > > > > > >   case UI_SET_UNIQ & ~IOCSIZE_MASK:
> > > > > > > > > > >
> > > > > > > > > > > as part of section /* Now check variable-length commands */
> > > > > > > > > >
> > > > > > > > > > If we did not have UI_SET_PHYS in its current form, I'd agree with you,
> > > > > > > > > > but I think there is benefit in having UI_SET_UNIQ be similar to
> > > > > > > > > > UI_SET_PHYS.
> > > > > > > > >
> > > > > > > > > I thought that ioctl is just number, so we can define it as we want. And
> > > > > > > > > because uinput.c has already switch for variable-length commands it
> > > > > > > > > would be easy to use it. Final handling can be in separate function like
> > > > > > > > > for UI_SET_PHYS which can look like same.
> > > > > > > >
> > > > > > > > Yes, we can define ioctl number as whatever we want. What I was trying
> > > > > > > > to say, right now users do this:
> > > > > > > >
> > > > > > > >     rc = ioctl(fd, UI_SET_PHYS, "whatever");
> > > > > > > >     ...
> > > > > > > >
> > > > > > > > and with UI_SET_UNIQ they expect the following to work:
> > > > > > > >
> > > > > > > >     rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > > > >     ...
> > > > > > >
> > > > > > > And would not following definition
> > > > > > >
> > > > > > >   #define UI_SET_UNIQ _IOW(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, 0)
> > > > > > >
> > > > > > > allow userspace to call
> > > > > > >
> > > > > > >   rc = ioctl(fd, UI_SET_UNIQ, "whatever");
> > > > > > >
> > > > > > > as you want?
> > > > > >
> > > > > > OK, so what you are saying is that we can have whatever in the size
> > > > > > portion of ioctl number and simply ignore it in the driver
> > > > >
> > > > > Yes.
> > > > >
> > > > > > (and I do not
> > > > > > think we need to do any of "UI_SET_UNIQ & ~IOCSIZE_MASK" really).
> > > > >
> > > > > You are right, we do not need to clear any IOCSIZE_MASK. As ioctl number
> > > > > would be always sam constant number. So it would be really simple. So
> > > > > original patch would work if UI_SET_UNIQ define would be changed to
> > > > > above with _IOW() macro.
> > > > >
> > > > > > While this would work, I am not sure it is the best option as I think
> > > > > > we'd have to comment extensively why we have arbitrary number in place
> > > > > > of the size.
> > > > >
> > > > > Comment can be added. But this is as ioctl is going to accept variable
> > > > > length array (not fixed array), zero value make sense for me (zero as we
> > > > > do not know exact size).
> > > > >
> > > > > > And we still do not really save anything, as we still have to go through
> > > > > > compat ioctl handler (since we have it already) and it is very simple to
> > > > > > add a case for UI_SET_UNIQ there...
> > > > >
> > > > > Yes, compat ioctl is still used. But my proposed solution does not
> > > > > involve to define a new compact ioctl number just for sizeof(char *).
> > > > >
> > > > > I'm looking at this particular problem from side, that there is no
> > > > > reason to define two new ioctl numbers for UI_SET_UNIQ (one normal
> > > > > number and one compat number), when one number is enough. It is one new
> > > > > ioctl call, so one ioctl number should be enough.
> > > > >
> > > > > And also with my proposed solution with ioctl size=0 it simplify
> > > > > implementation of UI_SET_UNIQ as it is not needed to implement also
> > > > > UI_SET_UNIQ_COMPAT ioctl nor touch compat ioct code path. Basically
> > > > > original patch (with changed UI_SET_UNIQ macro) is enough.
> > > > >
> > > > > But of of course, this is my view of this problem and I would not be
> > > > > against your decision from maintainer position. Both solutions would
> > > > > work correctly and bring same behavior for userspace applications.
> > > >
> > > >
> > > > Hi Dmitry!
> > > >
> > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > another argument why not specify 'char *' in _IOW:
> > > >
> > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > passed as pointer to the third argument for ioctl() syscall.
> > > >
> > > > So e.g.:
> > > >
> > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > >
> > > > is called from userspace as:
> > > >
> > > >   int val;
> > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > >
> > > > Or
> > > >
> > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > >
> > > > is called as:
> > > >
> > > >   struct input_mask val;
> > > >   ioctl(fd, EVIOCSMASK, &val);
> > > >
> > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > struct input_mask, but struct input_mask itself.
> > > >
> > > > And in case you define
> > > >
> > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > >
> > > > then you by above usage you should pass data as:
> > > >
> > > >   char *val = "DATA";
> > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > >
> > > > Which is not same as just:
> > > >
> > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > >
> > > > As in former case you passed pointer to pointer to data and in later
> > > > case you passed only pointer to data.
> > > >
> > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > also reason why compat ioctl for it was introduced.
> > >
> > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > what to do with all of this...
> > >
> > > Maybe we should define
> > >
> > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> >
> > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > so functions/macros do not take buffer length.
> Except strncpy, strndup, snprintf, etc. all expect a buffer length. At

This is something different as for these functions you pass buffer and
length of buffer which is used in write mode -- not for read mode.

> the user to kernel boundary of ioctl, I think we should require size
> of the user buffer regardless of the data type.
> 
> > _STR therefore in names looks inconsistent.
> The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> requiring the length seems to be common across various ioctls.
> * input.h requires a length when requesting the phys and uniq
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> * Same with HIDRAW when setting and getting features:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88

All these ioctls where is passed length are in opposite direction
(_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).

I fully agree that when you need to read something from kernel
(_IOC_READ) and then writing it to userspace, you need to specify length
of userspace buffer. Exactly same as with userspace functions like
memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
overflow as callee does not know length of buffer.

But here we we have there quite different problem, we need to write
something to kernel from userspace (_IOC_WRITE) and we are passing
nul-term string. So in this case specifying size is not required as it
is implicitly specified as part of passed string.

> >
> > Maybe this is something which should be handled and unified at global
> > scope of linux include files. Or al least make consensus how should be
> > new ioctls for linux written.
> >
> > Otherwise each API would use different ioctl schema and mess would be
> > still there. Which means that defining a new ioctl UI_SET_PHYS_STR for
> > existing one UI_SET_PHYS does not fix anything -- but rather make mess a
> > big larger.
> >
> > Or is there already some discussion on LKML about this ioctl problem?
> I found this fairly old email (couldn't find something more recent):
> http://lkml.iu.edu/hypermail/linux/kernel/9903.3/0325.html
> I think the intent is for userspace to provide the size of the string
> they're passing in (or at least the size of the allocated buffer that
> has the string).

Yes, but this is again opposite direction _IOC_READ. As wrote above I
agree that specifying size of buffer for filling data is required.

> >
> > > and mark UI_SET_PHYS as deprecated/wrong? This will allow us to specify
> > > exactly how much data kernel is supposed to fetch from userspace instead
> > > of trying to rely on a null-terminated string.
> > >
> > > It would also be very helpful if BlueZ did not accept changes that use
> > > this brand new ioctl until after we agreed on how it should look like.
> > > Luiz, can it be reverted for now please?
> > >
> > > Thanks.
> > >
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> 
> Abhishek

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Dmitry Torokhov]

On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > >
> > > > > Hi Dmitry!
> > > > >
> > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > another argument why not specify 'char *' in _IOW:
> > > > >
> > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > >
> > > > > So e.g.:
> > > > >
> > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > >
> > > > > is called from userspace as:
> > > > >
> > > > >   int val;
> > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > >
> > > > > Or
> > > > >
> > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > >
> > > > > is called as:
> > > > >
> > > > >   struct input_mask val;
> > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > >
> > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > struct input_mask, but struct input_mask itself.
> > > > >
> > > > > And in case you define
> > > > >
> > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > >
> > > > > then you by above usage you should pass data as:
> > > > >
> > > > >   char *val = "DATA";
> > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > >
> > > > > Which is not same as just:
> > > > >
> > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > >
> > > > > As in former case you passed pointer to pointer to data and in later
> > > > > case you passed only pointer to data.
> > > > >
> > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > also reason why compat ioctl for it was introduced.
> > > >
> > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > what to do with all of this...
> > > >
> > > > Maybe we should define
> > > >
> > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > >
> > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > so functions/macros do not take buffer length.
> > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> 
> This is something different as for these functions you pass buffer and
> length of buffer which is used in write mode -- not for read mode.
> 
> > the user to kernel boundary of ioctl, I think we should require size
> > of the user buffer regardless of the data type.
> > 
> > > _STR therefore in names looks inconsistent.
> > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > requiring the length seems to be common across various ioctls.
> > * input.h requires a length when requesting the phys and uniq
> > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > * Same with HIDRAW when setting and getting features:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> 
> All these ioctls where is passed length are in opposite direction
> (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> 
> I fully agree that when you need to read something from kernel
> (_IOC_READ) and then writing it to userspace, you need to specify length
> of userspace buffer. Exactly same as with userspace functions like
> memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> overflow as callee does not know length of buffer.
> 
> But here we we have there quite different problem, we need to write
> something to kernel from userspace (_IOC_WRITE) and we are passing
> nul-term string. So in this case specifying size is not required as it
> is implicitly specified as part of passed string.

With the new IOCTL definitions it does not need to be a NULL-terminated
string. It can be a buffer of characters with given length, and kernel
will NULL-terminate as this it what it wants, not what the caller has to
give.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Abhishek Pandit-Subedi]

Hi,

Since there hasn't been further activity on this thread, I assume that
we're agreed that requiring the size in the IOCTL is ok (i.e.
UI_SET_PHYS_STR(18)).

Dmitry, could you please review -- [PATCH v4] Input: uinput - Add
UI_SET_PHYS_STR and UI_SET_UNIQ_STR -- from December 4 for merge?

Thanks
Abhishek

On Fri, Dec 6, 2019 at 9:40 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >
> > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > >
> > > > > > Hi Dmitry!
> > > > > >
> > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > another argument why not specify 'char *' in _IOW:
> > > > > >
> > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > >
> > > > > > So e.g.:
> > > > > >
> > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > >
> > > > > > is called from userspace as:
> > > > > >
> > > > > >   int val;
> > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > >
> > > > > > Or
> > > > > >
> > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > >
> > > > > > is called as:
> > > > > >
> > > > > >   struct input_mask val;
> > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > >
> > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > struct input_mask, but struct input_mask itself.
> > > > > >
> > > > > > And in case you define
> > > > > >
> > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > >
> > > > > > then you by above usage you should pass data as:
> > > > > >
> > > > > >   char *val = "DATA";
> > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > >
> > > > > > Which is not same as just:
> > > > > >
> > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > >
> > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > case you passed only pointer to data.
> > > > > >
> > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > also reason why compat ioctl for it was introduced.
> > > > >
> > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > what to do with all of this...
> > > > >
> > > > > Maybe we should define
> > > > >
> > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > >
> > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > so functions/macros do not take buffer length.
> > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> >
> > This is something different as for these functions you pass buffer and
> > length of buffer which is used in write mode -- not for read mode.
> >
> > > the user to kernel boundary of ioctl, I think we should require size
> > > of the user buffer regardless of the data type.
> > >
> > > > _STR therefore in names looks inconsistent.
> > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > requiring the length seems to be common across various ioctls.
> > > * input.h requires a length when requesting the phys and uniq
> > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > * Same with HIDRAW when setting and getting features:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> >
> > All these ioctls where is passed length are in opposite direction
> > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> >
> > I fully agree that when you need to read something from kernel
> > (_IOC_READ) and then writing it to userspace, you need to specify length
> > of userspace buffer. Exactly same as with userspace functions like
> > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > overflow as callee does not know length of buffer.
> >
> > But here we we have there quite different problem, we need to write
> > something to kernel from userspace (_IOC_WRITE) and we are passing
> > nul-term string. So in this case specifying size is not required as it
> > is implicitly specified as part of passed string.
>
> With the new IOCTL definitions it does not need to be a NULL-terminated
> string. It can be a buffer of characters with given length, and kernel
> will NULL-terminate as this it what it wants, not what the caller has to
> give.
>
> Thanks.
>
> --
> Dmitry

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 6963 bytes --]

On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > >
> > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > >
> > > > > > Hi Dmitry!
> > > > > >
> > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > another argument why not specify 'char *' in _IOW:
> > > > > >
> > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > >
> > > > > > So e.g.:
> > > > > >
> > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > >
> > > > > > is called from userspace as:
> > > > > >
> > > > > >   int val;
> > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > >
> > > > > > Or
> > > > > >
> > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > >
> > > > > > is called as:
> > > > > >
> > > > > >   struct input_mask val;
> > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > >
> > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > struct input_mask, but struct input_mask itself.
> > > > > >
> > > > > > And in case you define
> > > > > >
> > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > >
> > > > > > then you by above usage you should pass data as:
> > > > > >
> > > > > >   char *val = "DATA";
> > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > >
> > > > > > Which is not same as just:
> > > > > >
> > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > >
> > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > case you passed only pointer to data.
> > > > > >
> > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > also reason why compat ioctl for it was introduced.
> > > > >
> > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > what to do with all of this...
> > > > >
> > > > > Maybe we should define
> > > > >
> > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > >
> > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > so functions/macros do not take buffer length.
> > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > 
> > This is something different as for these functions you pass buffer and
> > length of buffer which is used in write mode -- not for read mode.
> > 
> > > the user to kernel boundary of ioctl, I think we should require size
> > > of the user buffer regardless of the data type.
> > > 
> > > > _STR therefore in names looks inconsistent.
> > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > requiring the length seems to be common across various ioctls.
> > > * input.h requires a length when requesting the phys and uniq
> > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > * Same with HIDRAW when setting and getting features:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > 
> > All these ioctls where is passed length are in opposite direction
> > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > 
> > I fully agree that when you need to read something from kernel
> > (_IOC_READ) and then writing it to userspace, you need to specify length
> > of userspace buffer. Exactly same as with userspace functions like
> > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > overflow as callee does not know length of buffer.
> > 
> > But here we we have there quite different problem, we need to write
> > something to kernel from userspace (_IOC_WRITE) and we are passing
> > nul-term string. So in this case specifying size is not required as it
> > is implicitly specified as part of passed string.
> 
> With the new IOCTL definitions it does not need to be a NULL-terminated
> string. It can be a buffer of characters with given length, and kernel
> will NULL-terminate as this it what it wants, not what the caller has to
> give.

Hi Dmitry! I was thinking more about this problem and I will propose
alternative solution, but first with details...

I think that we should use NULL terminated strings. Or better disallow
NULL byte inside string. Reason: all userspace application expects that
input device name would be NULL terminated which implies that in the
middle of name cannot be NULL byte.

So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
ioctl API to use buffer + size (with upper bound limit for size) instead
of passing NULL term string (with upper bound limit for string size)
then kernel have to add a leading NULL byte to string, plus check that
in the buffer there is no NULL byte. I guess this a very little
complicate code, but nothing which is problematic.

And on the userspace part. Now when userspace want to pass constant
string for device name, it just call

  ioctl(fd, UI_SET_PHYS, "my name of device");

After adding a new ioctl with buffer + size API, userspace would have to
call:

  ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");

which looks strange, so programmers would had to move device name into
new variable:

  const char *name = "my name of device";
  ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);

For me the old ioctl API looks easier to use (no need for strlen() or
extra variable), but this is just my preference of usage -- as it is
simpler for me. Maybe you would have different opinion...

And now question: Why we have uinput_compat_ioctl()? It is there only
because size part of IOCTL number is different on 32bit and 64bit
systems. As we know size part of UI_SET_PHYS is wrong and does not make
sense...

Would not it be better to change size of UI_SET_PHYS to just zero and
then when matching ioctl number just ignore size for this UI_SET_PHYS
ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
added in: https://git.kernel.org/tip/tip/c/7c7da40

And we would not have to deal with uinput_compat_ioctl() at all.

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

[-- Attachment #1: Type: text/plain, Size: 10771 bytes --]

On Wednesday 18 December 2019 12:02:24 Pali Rohár wrote:
> On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> > On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > >
> > > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > > >
> > > > > > > Hi Dmitry!
> > > > > > >
> > > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > > another argument why not specify 'char *' in _IOW:
> > > > > > >
> > > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > > >
> > > > > > > So e.g.:
> > > > > > >
> > > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > > >
> > > > > > > is called from userspace as:
> > > > > > >
> > > > > > >   int val;
> > > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > > >
> > > > > > > Or
> > > > > > >
> > > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > > >
> > > > > > > is called as:
> > > > > > >
> > > > > > >   struct input_mask val;
> > > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > > >
> > > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > > struct input_mask, but struct input_mask itself.
> > > > > > >
> > > > > > > And in case you define
> > > > > > >
> > > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > > >
> > > > > > > then you by above usage you should pass data as:
> > > > > > >
> > > > > > >   char *val = "DATA";
> > > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > > >
> > > > > > > Which is not same as just:
> > > > > > >
> > > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > > >
> > > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > > case you passed only pointer to data.
> > > > > > >
> > > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > > also reason why compat ioctl for it was introduced.
> > > > > >
> > > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > > what to do with all of this...
> > > > > >
> > > > > > Maybe we should define
> > > > > >
> > > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > > >
> > > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > > so functions/macros do not take buffer length.
> > > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > > 
> > > This is something different as for these functions you pass buffer and
> > > length of buffer which is used in write mode -- not for read mode.
> > > 
> > > > the user to kernel boundary of ioctl, I think we should require size
> > > > of the user buffer regardless of the data type.
> > > > 
> > > > > _STR therefore in names looks inconsistent.
> > > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > > requiring the length seems to be common across various ioctls.
> > > > * input.h requires a length when requesting the phys and uniq
> > > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > > * Same with HIDRAW when setting and getting features:
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > > 
> > > All these ioctls where is passed length are in opposite direction
> > > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > > 
> > > I fully agree that when you need to read something from kernel
> > > (_IOC_READ) and then writing it to userspace, you need to specify length
> > > of userspace buffer. Exactly same as with userspace functions like
> > > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > > overflow as callee does not know length of buffer.
> > > 
> > > But here we we have there quite different problem, we need to write
> > > something to kernel from userspace (_IOC_WRITE) and we are passing
> > > nul-term string. So in this case specifying size is not required as it
> > > is implicitly specified as part of passed string.
> > 
> > With the new IOCTL definitions it does not need to be a NULL-terminated
> > string. It can be a buffer of characters with given length, and kernel
> > will NULL-terminate as this it what it wants, not what the caller has to
> > give.
> 
> Hi Dmitry! I was thinking more about this problem and I will propose
> alternative solution, but first with details...
> 
> I think that we should use NULL terminated strings. Or better disallow
> NULL byte inside string. Reason: all userspace application expects that
> input device name would be NULL terminated which implies that in the
> middle of name cannot be NULL byte.
> 
> So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
> ioctl API to use buffer + size (with upper bound limit for size) instead
> of passing NULL term string (with upper bound limit for string size)
> then kernel have to add a leading NULL byte to string, plus check that
> in the buffer there is no NULL byte. I guess this a very little
> complicate code, but nothing which is problematic.
> 
> And on the userspace part. Now when userspace want to pass constant
> string for device name, it just call
> 
>   ioctl(fd, UI_SET_PHYS, "my name of device");
> 
> After adding a new ioctl with buffer + size API, userspace would have to
> call:
> 
>   ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");
> 
> which looks strange, so programmers would had to move device name into
> new variable:
> 
>   const char *name = "my name of device";
>   ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);
> 
> For me the old ioctl API looks easier to use (no need for strlen() or
> extra variable), but this is just my preference of usage -- as it is
> simpler for me. Maybe you would have different opinion...
> 
> And now question: Why we have uinput_compat_ioctl()? It is there only
> because size part of IOCTL number is different on 32bit and 64bit
> systems. As we know size part of UI_SET_PHYS is wrong and does not make
> sense...
> 
> Would not it be better to change size of UI_SET_PHYS to just zero and
> then when matching ioctl number just ignore size for this UI_SET_PHYS
> ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
> added in: https://git.kernel.org/tip/tip/c/7c7da40
> 
> And we would not have to deal with uinput_compat_ioctl() at all.

Below is example how change for removing UI_SET_PHYS_COMPAT may look
like. As header file is not changed and UI_SET_PHYS accepts any size
argument, it therefore accept also 32bit and 64bit integer. So no
existing 32bit applications which use UI_SET_PHYS on 64bit kernel would
not be broken...

Is not this better change then introducing a new UI_SET_PHYS_STR ioctl
number? Because introduction of new IOCTL number has one big
disadvantage: Userspace applications needs to support fallback to old
number as older versions of kernels would be in use for a long time. And
because kernel does not have to remove old IOCTL number for backward
compatibility there is basically no need for userspace application to
user new UI_SET_PHYS_STR IOCTL number...

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index fd253781b..b645210d5 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -915,22 +915,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 		retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
 		goto out;
 
-	case UI_SET_PHYS:
-		if (udev->state == UIST_CREATED) {
-			retval = -EINVAL;
-			goto out;
-		}
-
-		phys = strndup_user(p, 1024);
-		if (IS_ERR(phys)) {
-			retval = PTR_ERR(phys);
-			goto out;
-		}
-
-		kfree(udev->dev->phys);
-		udev->dev->phys = phys;
-		goto out;
-
 	case UI_BEGIN_FF_UPLOAD:
 		retval = uinput_ff_upload_from_user(p, &ff_up);
 		if (retval)
@@ -1023,6 +1007,22 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
 	case UI_ABS_SETUP & ~IOCSIZE_MASK:
 		retval = uinput_abs_setup(udev, p, size);
 		goto out;
+
+	case UI_SET_PHYS & ~IOCSIZE_MASK:
+		if (udev->state == UIST_CREATED) {
+			retval = -EINVAL;
+			goto out;
+		}
+
+		phys = strndup_user(p, 1024);
+		if (IS_ERR(phys)) {
+			retval = PTR_ERR(phys);
+			goto out;
+		}
+
+		kfree(udev->dev->phys);
+		udev->dev->phys = phys;
+		goto out;
 	}
 
 	retval = -EINVAL;
@@ -1042,8 +1042,6 @@ static long uinput_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
  * These IOCTLs change their size and thus their numbers between
  * 32 and 64 bits.
  */
-#define UI_SET_PHYS_COMPAT		\
-	_IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
 #define UI_BEGIN_FF_UPLOAD_COMPAT	\
 	_IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload_compat)
 #define UI_END_FF_UPLOAD_COMPAT		\
@@ -1053,9 +1051,6 @@ static long uinput_compat_ioctl(struct file *file,
 				unsigned int cmd, unsigned long arg)
 {
 	switch (cmd) {
-	case UI_SET_PHYS_COMPAT:
-		cmd = UI_SET_PHYS;
-		break;
 	case UI_BEGIN_FF_UPLOAD_COMPAT:
 		cmd = UI_BEGIN_FF_UPLOAD;
 		break;
diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
index c9e677e3a..6bda2a142 100644
--- a/include/uapi/linux/uinput.h
+++ b/include/uapi/linux/uinput.h
@@ -142,6 +142,8 @@ struct uinput_abs_setup {
 #define UI_SET_LEDBIT		_IOW(UINPUT_IOCTL_BASE, 105, int)
 #define UI_SET_SNDBIT		_IOW(UINPUT_IOCTL_BASE, 106, int)
 #define UI_SET_FFBIT		_IOW(UINPUT_IOCTL_BASE, 107, int)
+/* Argument is nul-term string and for backward compatibility is there
+ * specified char*, but size argument (char *) is ignored by this ioctl */
 #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
 #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
 #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)


-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

On Wednesday 18 December 2019 12:26:59 Pali Rohár wrote:
> On Wednesday 18 December 2019 12:02:24 Pali Rohár wrote:
> > On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> > > On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > > > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > >
> > > > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > > > >
> > > > > > > > Hi Dmitry!
> > > > > > > >
> > > > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > > > another argument why not specify 'char *' in _IOW:
> > > > > > > >
> > > > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > > > >
> > > > > > > > So e.g.:
> > > > > > > >
> > > > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > > > >
> > > > > > > > is called from userspace as:
> > > > > > > >
> > > > > > > >   int val;
> > > > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > > > >
> > > > > > > > Or
> > > > > > > >
> > > > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > > > >
> > > > > > > > is called as:
> > > > > > > >
> > > > > > > >   struct input_mask val;
> > > > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > > > >
> > > > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > > > struct input_mask, but struct input_mask itself.
> > > > > > > >
> > > > > > > > And in case you define
> > > > > > > >
> > > > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > > > >
> > > > > > > > then you by above usage you should pass data as:
> > > > > > > >
> > > > > > > >   char *val = "DATA";
> > > > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > > > >
> > > > > > > > Which is not same as just:
> > > > > > > >
> > > > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > > > >
> > > > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > > > case you passed only pointer to data.
> > > > > > > >
> > > > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > > > also reason why compat ioctl for it was introduced.
> > > > > > >
> > > > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > > > what to do with all of this...
> > > > > > >
> > > > > > > Maybe we should define
> > > > > > >
> > > > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > > > >
> > > > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > > > so functions/macros do not take buffer length.
> > > > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > > > 
> > > > This is something different as for these functions you pass buffer and
> > > > length of buffer which is used in write mode -- not for read mode.
> > > > 
> > > > > the user to kernel boundary of ioctl, I think we should require size
> > > > > of the user buffer regardless of the data type.
> > > > > 
> > > > > > _STR therefore in names looks inconsistent.
> > > > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > > > requiring the length seems to be common across various ioctls.
> > > > > * input.h requires a length when requesting the phys and uniq
> > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > > > * Same with HIDRAW when setting and getting features:
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > > > 
> > > > All these ioctls where is passed length are in opposite direction
> > > > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > > > 
> > > > I fully agree that when you need to read something from kernel
> > > > (_IOC_READ) and then writing it to userspace, you need to specify length
> > > > of userspace buffer. Exactly same as with userspace functions like
> > > > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > > > overflow as callee does not know length of buffer.
> > > > 
> > > > But here we we have there quite different problem, we need to write
> > > > something to kernel from userspace (_IOC_WRITE) and we are passing
> > > > nul-term string. So in this case specifying size is not required as it
> > > > is implicitly specified as part of passed string.
> > > 
> > > With the new IOCTL definitions it does not need to be a NULL-terminated
> > > string. It can be a buffer of characters with given length, and kernel
> > > will NULL-terminate as this it what it wants, not what the caller has to
> > > give.
> > 
> > Hi Dmitry! I was thinking more about this problem and I will propose
> > alternative solution, but first with details...
> > 
> > I think that we should use NULL terminated strings. Or better disallow
> > NULL byte inside string. Reason: all userspace application expects that
> > input device name would be NULL terminated which implies that in the
> > middle of name cannot be NULL byte.
> > 
> > So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
> > ioctl API to use buffer + size (with upper bound limit for size) instead
> > of passing NULL term string (with upper bound limit for string size)
> > then kernel have to add a leading NULL byte to string, plus check that
> > in the buffer there is no NULL byte. I guess this a very little
> > complicate code, but nothing which is problematic.
> > 
> > And on the userspace part. Now when userspace want to pass constant
> > string for device name, it just call
> > 
> >   ioctl(fd, UI_SET_PHYS, "my name of device");
> > 
> > After adding a new ioctl with buffer + size API, userspace would have to
> > call:
> > 
> >   ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");
> > 
> > which looks strange, so programmers would had to move device name into
> > new variable:
> > 
> >   const char *name = "my name of device";
> >   ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);
> > 
> > For me the old ioctl API looks easier to use (no need for strlen() or
> > extra variable), but this is just my preference of usage -- as it is
> > simpler for me. Maybe you would have different opinion...
> > 
> > And now question: Why we have uinput_compat_ioctl()? It is there only
> > because size part of IOCTL number is different on 32bit and 64bit
> > systems. As we know size part of UI_SET_PHYS is wrong and does not make
> > sense...
> > 
> > Would not it be better to change size of UI_SET_PHYS to just zero and
> > then when matching ioctl number just ignore size for this UI_SET_PHYS
> > ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
> > added in: https://git.kernel.org/tip/tip/c/7c7da40
> > 
> > And we would not have to deal with uinput_compat_ioctl() at all.
> 
> Below is example how change for removing UI_SET_PHYS_COMPAT may look
> like. As header file is not changed and UI_SET_PHYS accepts any size
> argument, it therefore accept also 32bit and 64bit integer. So no
> existing 32bit applications which use UI_SET_PHYS on 64bit kernel would
> not be broken...
> 
> Is not this better change then introducing a new UI_SET_PHYS_STR ioctl
> number? Because introduction of new IOCTL number has one big
> disadvantage: Userspace applications needs to support fallback to old
> number as older versions of kernels would be in use for a long time. And
> because kernel does not have to remove old IOCTL number for backward
> compatibility there is basically no need for userspace application to
> user new UI_SET_PHYS_STR IOCTL number...

Hello! I would like to remind this discussion as problem around a new
UI_SET_UNIQ ioctl is not solved yet and uniq property is really useful
for e.g. bluetooth (uinput) devices.

Dmitry, when you have a time, could you please look at this discussion
and decide how to go ahead?

> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index fd253781b..b645210d5 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -915,22 +915,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  		retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
>  		goto out;
>  
> -	case UI_SET_PHYS:
> -		if (udev->state == UIST_CREATED) {
> -			retval = -EINVAL;
> -			goto out;
> -		}
> -
> -		phys = strndup_user(p, 1024);
> -		if (IS_ERR(phys)) {
> -			retval = PTR_ERR(phys);
> -			goto out;
> -		}
> -
> -		kfree(udev->dev->phys);
> -		udev->dev->phys = phys;
> -		goto out;
> -
>  	case UI_BEGIN_FF_UPLOAD:
>  		retval = uinput_ff_upload_from_user(p, &ff_up);
>  		if (retval)
> @@ -1023,6 +1007,22 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  	case UI_ABS_SETUP & ~IOCSIZE_MASK:
>  		retval = uinput_abs_setup(udev, p, size);
>  		goto out;
> +
> +	case UI_SET_PHYS & ~IOCSIZE_MASK:
> +		if (udev->state == UIST_CREATED) {
> +			retval = -EINVAL;
> +			goto out;
> +		}
> +
> +		phys = strndup_user(p, 1024);
> +		if (IS_ERR(phys)) {
> +			retval = PTR_ERR(phys);
> +			goto out;
> +		}
> +
> +		kfree(udev->dev->phys);
> +		udev->dev->phys = phys;
> +		goto out;
>  	}
>  
>  	retval = -EINVAL;
> @@ -1042,8 +1042,6 @@ static long uinput_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
>   * These IOCTLs change their size and thus their numbers between
>   * 32 and 64 bits.
>   */
> -#define UI_SET_PHYS_COMPAT		\
> -	_IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
>  #define UI_BEGIN_FF_UPLOAD_COMPAT	\
>  	_IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload_compat)
>  #define UI_END_FF_UPLOAD_COMPAT		\
> @@ -1053,9 +1051,6 @@ static long uinput_compat_ioctl(struct file *file,
>  				unsigned int cmd, unsigned long arg)
>  {
>  	switch (cmd) {
> -	case UI_SET_PHYS_COMPAT:
> -		cmd = UI_SET_PHYS;
> -		break;
>  	case UI_BEGIN_FF_UPLOAD_COMPAT:
>  		cmd = UI_BEGIN_FF_UPLOAD;
>  		break;
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c9e677e3a..6bda2a142 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -142,6 +142,8 @@ struct uinput_abs_setup {
>  #define UI_SET_LEDBIT		_IOW(UINPUT_IOCTL_BASE, 105, int)
>  #define UI_SET_SNDBIT		_IOW(UINPUT_IOCTL_BASE, 106, int)
>  #define UI_SET_FFBIT		_IOW(UINPUT_IOCTL_BASE, 107, int)
> +/* Argument is nul-term string and for backward compatibility is there
> + * specified char*, but size argument (char *) is ignored by this ioctl */
>  #define UI_SET_PHYS		_IOW(UINPUT_IOCTL_BASE, 108, char*)
>  #define UI_SET_SWBIT		_IOW(UINPUT_IOCTL_BASE, 109, int)
>  #define UI_SET_PROPBIT		_IOW(UINPUT_IOCTL_BASE, 110, int)
> 
> 

-- 
Pali Rohár
pali.rohar@gmail.com

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Luiz Augusto von Dentz]

Hi,

On Sun, Mar 22, 2020 at 8:47 AM Pali Rohár <pali.rohar@gmail.com> wrote:
>
> On Wednesday 18 December 2019 12:26:59 Pali Rohár wrote:
> > On Wednesday 18 December 2019 12:02:24 Pali Rohár wrote:
> > > On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> > > > On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > > > > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > > > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > > > > >
> > > > > > > > > Hi Dmitry!
> > > > > > > > >
> > > > > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > > > > another argument why not specify 'char *' in _IOW:
> > > > > > > > >
> > > > > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > > > > >
> > > > > > > > > So e.g.:
> > > > > > > > >
> > > > > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > > > > >
> > > > > > > > > is called from userspace as:
> > > > > > > > >
> > > > > > > > >   int val;
> > > > > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > > > > >
> > > > > > > > > Or
> > > > > > > > >
> > > > > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > > > > >
> > > > > > > > > is called as:
> > > > > > > > >
> > > > > > > > >   struct input_mask val;
> > > > > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > > > > >
> > > > > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > > > > struct input_mask, but struct input_mask itself.
> > > > > > > > >
> > > > > > > > > And in case you define
> > > > > > > > >
> > > > > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > > > > >
> > > > > > > > > then you by above usage you should pass data as:
> > > > > > > > >
> > > > > > > > >   char *val = "DATA";
> > > > > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > > > > >
> > > > > > > > > Which is not same as just:
> > > > > > > > >
> > > > > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > > > > >
> > > > > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > > > > case you passed only pointer to data.
> > > > > > > > >
> > > > > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > > > > also reason why compat ioctl for it was introduced.
> > > > > > > >
> > > > > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > > > > what to do with all of this...
> > > > > > > >
> > > > > > > > Maybe we should define
> > > > > > > >
> > > > > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > > > > >
> > > > > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > > > > so functions/macros do not take buffer length.
> > > > > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > > > >
> > > > > This is something different as for these functions you pass buffer and
> > > > > length of buffer which is used in write mode -- not for read mode.
> > > > >
> > > > > > the user to kernel boundary of ioctl, I think we should require size
> > > > > > of the user buffer regardless of the data type.
> > > > > >
> > > > > > > _STR therefore in names looks inconsistent.
> > > > > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > > > > requiring the length seems to be common across various ioctls.
> > > > > > * input.h requires a length when requesting the phys and uniq
> > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > > > > * Same with HIDRAW when setting and getting features:
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > > > >
> > > > > All these ioctls where is passed length are in opposite direction
> > > > > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > > > >
> > > > > I fully agree that when you need to read something from kernel
> > > > > (_IOC_READ) and then writing it to userspace, you need to specify length
> > > > > of userspace buffer. Exactly same as with userspace functions like
> > > > > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > > > > overflow as callee does not know length of buffer.
> > > > >
> > > > > But here we we have there quite different problem, we need to write
> > > > > something to kernel from userspace (_IOC_WRITE) and we are passing
> > > > > nul-term string. So in this case specifying size is not required as it
> > > > > is implicitly specified as part of passed string.
> > > >
> > > > With the new IOCTL definitions it does not need to be a NULL-terminated
> > > > string. It can be a buffer of characters with given length, and kernel
> > > > will NULL-terminate as this it what it wants, not what the caller has to
> > > > give.
> > >
> > > Hi Dmitry! I was thinking more about this problem and I will propose
> > > alternative solution, but first with details...
> > >
> > > I think that we should use NULL terminated strings. Or better disallow
> > > NULL byte inside string. Reason: all userspace application expects that
> > > input device name would be NULL terminated which implies that in the
> > > middle of name cannot be NULL byte.
> > >
> > > So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
> > > ioctl API to use buffer + size (with upper bound limit for size) instead
> > > of passing NULL term string (with upper bound limit for string size)
> > > then kernel have to add a leading NULL byte to string, plus check that
> > > in the buffer there is no NULL byte. I guess this a very little
> > > complicate code, but nothing which is problematic.
> > >
> > > And on the userspace part. Now when userspace want to pass constant
> > > string for device name, it just call
> > >
> > >   ioctl(fd, UI_SET_PHYS, "my name of device");
> > >
> > > After adding a new ioctl with buffer + size API, userspace would have to
> > > call:
> > >
> > >   ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");
> > >
> > > which looks strange, so programmers would had to move device name into
> > > new variable:
> > >
> > >   const char *name = "my name of device";
> > >   ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);
> > >
> > > For me the old ioctl API looks easier to use (no need for strlen() or
> > > extra variable), but this is just my preference of usage -- as it is
> > > simpler for me. Maybe you would have different opinion...
> > >
> > > And now question: Why we have uinput_compat_ioctl()? It is there only
> > > because size part of IOCTL number is different on 32bit and 64bit
> > > systems. As we know size part of UI_SET_PHYS is wrong and does not make
> > > sense...
> > >
> > > Would not it be better to change size of UI_SET_PHYS to just zero and
> > > then when matching ioctl number just ignore size for this UI_SET_PHYS
> > > ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
> > > added in: https://git.kernel.org/tip/tip/c/7c7da40
> > >
> > > And we would not have to deal with uinput_compat_ioctl() at all.
> >
> > Below is example how change for removing UI_SET_PHYS_COMPAT may look
> > like. As header file is not changed and UI_SET_PHYS accepts any size
> > argument, it therefore accept also 32bit and 64bit integer. So no
> > existing 32bit applications which use UI_SET_PHYS on 64bit kernel would
> > not be broken...
> >
> > Is not this better change then introducing a new UI_SET_PHYS_STR ioctl
> > number? Because introduction of new IOCTL number has one big
> > disadvantage: Userspace applications needs to support fallback to old
> > number as older versions of kernels would be in use for a long time. And
> > because kernel does not have to remove old IOCTL number for backward
> > compatibility there is basically no need for userspace application to
> > user new UI_SET_PHYS_STR IOCTL number...
>
> Hello! I would like to remind this discussion as problem around a new
> UI_SET_UNIQ ioctl is not solved yet and uniq property is really useful
> for e.g. bluetooth (uinput) devices.
>
> Dmitry, when you have a time, could you please look at this discussion
> and decide how to go ahead?

Have we decided not to move further with these changes? I actually
have a bug in BlueZ related to it since right now we are inconsistent
with respect to how we handle uhid vs uinput:

https://github.com/bluez/bluez/issues/352

> > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > index fd253781b..b645210d5 100644
> > --- a/drivers/input/misc/uinput.c
> > +++ b/drivers/input/misc/uinput.c
> > @@ -915,22 +915,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> >               retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
> >               goto out;
> >
> > -     case UI_SET_PHYS:
> > -             if (udev->state == UIST_CREATED) {
> > -                     retval = -EINVAL;
> > -                     goto out;
> > -             }
> > -
> > -             phys = strndup_user(p, 1024);
> > -             if (IS_ERR(phys)) {
> > -                     retval = PTR_ERR(phys);
> > -                     goto out;
> > -             }
> > -
> > -             kfree(udev->dev->phys);
> > -             udev->dev->phys = phys;
> > -             goto out;
> > -
> >       case UI_BEGIN_FF_UPLOAD:
> >               retval = uinput_ff_upload_from_user(p, &ff_up);
> >               if (retval)
> > @@ -1023,6 +1007,22 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> >       case UI_ABS_SETUP & ~IOCSIZE_MASK:
> >               retval = uinput_abs_setup(udev, p, size);
> >               goto out;
> > +
> > +     case UI_SET_PHYS & ~IOCSIZE_MASK:
> > +             if (udev->state == UIST_CREATED) {
> > +                     retval = -EINVAL;
> > +                     goto out;
> > +             }
> > +
> > +             phys = strndup_user(p, 1024);
> > +             if (IS_ERR(phys)) {
> > +                     retval = PTR_ERR(phys);
> > +                     goto out;
> > +             }
> > +
> > +             kfree(udev->dev->phys);
> > +             udev->dev->phys = phys;
> > +             goto out;
> >       }
> >
> >       retval = -EINVAL;
> > @@ -1042,8 +1042,6 @@ static long uinput_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> >   * These IOCTLs change their size and thus their numbers between
> >   * 32 and 64 bits.
> >   */
> > -#define UI_SET_PHYS_COMPAT           \
> > -     _IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
> >  #define UI_BEGIN_FF_UPLOAD_COMPAT    \
> >       _IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload_compat)
> >  #define UI_END_FF_UPLOAD_COMPAT              \
> > @@ -1053,9 +1051,6 @@ static long uinput_compat_ioctl(struct file *file,
> >                               unsigned int cmd, unsigned long arg)
> >  {
> >       switch (cmd) {
> > -     case UI_SET_PHYS_COMPAT:
> > -             cmd = UI_SET_PHYS;
> > -             break;
> >       case UI_BEGIN_FF_UPLOAD_COMPAT:
> >               cmd = UI_BEGIN_FF_UPLOAD;
> >               break;
> > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > index c9e677e3a..6bda2a142 100644
> > --- a/include/uapi/linux/uinput.h
> > +++ b/include/uapi/linux/uinput.h
> > @@ -142,6 +142,8 @@ struct uinput_abs_setup {
> >  #define UI_SET_LEDBIT                _IOW(UINPUT_IOCTL_BASE, 105, int)
> >  #define UI_SET_SNDBIT                _IOW(UINPUT_IOCTL_BASE, 106, int)
> >  #define UI_SET_FFBIT         _IOW(UINPUT_IOCTL_BASE, 107, int)
> > +/* Argument is nul-term string and for backward compatibility is there
> > + * specified char*, but size argument (char *) is ignored by this ioctl */
> >  #define UI_SET_PHYS          _IOW(UINPUT_IOCTL_BASE, 108, char*)
> >  #define UI_SET_SWBIT         _IOW(UINPUT_IOCTL_BASE, 109, int)
> >  #define UI_SET_PROPBIT               _IOW(UINPUT_IOCTL_BASE, 110, int)
> >
> >
>
> --
> Pali Rohár
> pali.rohar@gmail.com



-- 
Luiz Augusto von Dentz

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[macmpi]

Hi!

Bug 216134 has been been submitted on this issue 
https://bugzilla.kernel.org/show_bug.cgi?id=216134

Thanks for consideration and feedback.

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Chris Morgan]

On Mon, Jun 13, 2022 at 02:36:18PM -0700, Luiz Augusto von Dentz wrote:
> Hi,
> 
> On Sun, Mar 22, 2020 at 8:47 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> >
> > On Wednesday 18 December 2019 12:26:59 Pali Rohár wrote:
> > > On Wednesday 18 December 2019 12:02:24 Pali Rohár wrote:
> > > > On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> > > > > On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > > > > > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > > > > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Dmitry!
> > > > > > > > > >
> > > > > > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > > > > > another argument why not specify 'char *' in _IOW:
> > > > > > > > > >
> > > > > > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > > > > > >
> > > > > > > > > > So e.g.:
> > > > > > > > > >
> > > > > > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > > > > > >
> > > > > > > > > > is called from userspace as:
> > > > > > > > > >
> > > > > > > > > >   int val;
> > > > > > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > > > > > >
> > > > > > > > > > Or
> > > > > > > > > >
> > > > > > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > > > > > >
> > > > > > > > > > is called as:
> > > > > > > > > >
> > > > > > > > > >   struct input_mask val;
> > > > > > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > > > > > >
> > > > > > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > > > > > struct input_mask, but struct input_mask itself.
> > > > > > > > > >
> > > > > > > > > > And in case you define
> > > > > > > > > >
> > > > > > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > > > > > >
> > > > > > > > > > then you by above usage you should pass data as:
> > > > > > > > > >
> > > > > > > > > >   char *val = "DATA";
> > > > > > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > > > > > >
> > > > > > > > > > Which is not same as just:
> > > > > > > > > >
> > > > > > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > > > > > >
> > > > > > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > > > > > case you passed only pointer to data.
> > > > > > > > > >
> > > > > > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > > > > > also reason why compat ioctl for it was introduced.
> > > > > > > > >
> > > > > > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > > > > > what to do with all of this...
> > > > > > > > >
> > > > > > > > > Maybe we should define
> > > > > > > > >
> > > > > > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > > > > > >
> > > > > > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > > > > > so functions/macros do not take buffer length.
> > > > > > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > > > > >
> > > > > > This is something different as for these functions you pass buffer and
> > > > > > length of buffer which is used in write mode -- not for read mode.
> > > > > >
> > > > > > > the user to kernel boundary of ioctl, I think we should require size
> > > > > > > of the user buffer regardless of the data type.
> > > > > > >
> > > > > > > > _STR therefore in names looks inconsistent.
> > > > > > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > > > > > requiring the length seems to be common across various ioctls.
> > > > > > > * input.h requires a length when requesting the phys and uniq
> > > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > > > > > * Same with HIDRAW when setting and getting features:
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > > > > >
> > > > > > All these ioctls where is passed length are in opposite direction
> > > > > > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > > > > >
> > > > > > I fully agree that when you need to read something from kernel
> > > > > > (_IOC_READ) and then writing it to userspace, you need to specify length
> > > > > > of userspace buffer. Exactly same as with userspace functions like
> > > > > > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > > > > > overflow as callee does not know length of buffer.
> > > > > >
> > > > > > But here we we have there quite different problem, we need to write
> > > > > > something to kernel from userspace (_IOC_WRITE) and we are passing
> > > > > > nul-term string. So in this case specifying size is not required as it
> > > > > > is implicitly specified as part of passed string.
> > > > >
> > > > > With the new IOCTL definitions it does not need to be a NULL-terminated
> > > > > string. It can be a buffer of characters with given length, and kernel
> > > > > will NULL-terminate as this it what it wants, not what the caller has to
> > > > > give.
> > > >
> > > > Hi Dmitry! I was thinking more about this problem and I will propose
> > > > alternative solution, but first with details...
> > > >
> > > > I think that we should use NULL terminated strings. Or better disallow
> > > > NULL byte inside string. Reason: all userspace application expects that
> > > > input device name would be NULL terminated which implies that in the
> > > > middle of name cannot be NULL byte.
> > > >
> > > > So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
> > > > ioctl API to use buffer + size (with upper bound limit for size) instead
> > > > of passing NULL term string (with upper bound limit for string size)
> > > > then kernel have to add a leading NULL byte to string, plus check that
> > > > in the buffer there is no NULL byte. I guess this a very little
> > > > complicate code, but nothing which is problematic.
> > > >
> > > > And on the userspace part. Now when userspace want to pass constant
> > > > string for device name, it just call
> > > >
> > > >   ioctl(fd, UI_SET_PHYS, "my name of device");
> > > >
> > > > After adding a new ioctl with buffer + size API, userspace would have to
> > > > call:
> > > >
> > > >   ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");
> > > >
> > > > which looks strange, so programmers would had to move device name into
> > > > new variable:
> > > >
> > > >   const char *name = "my name of device";
> > > >   ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);
> > > >
> > > > For me the old ioctl API looks easier to use (no need for strlen() or
> > > > extra variable), but this is just my preference of usage -- as it is
> > > > simpler for me. Maybe you would have different opinion...
> > > >
> > > > And now question: Why we have uinput_compat_ioctl()? It is there only
> > > > because size part of IOCTL number is different on 32bit and 64bit
> > > > systems. As we know size part of UI_SET_PHYS is wrong and does not make
> > > > sense...
> > > >
> > > > Would not it be better to change size of UI_SET_PHYS to just zero and
> > > > then when matching ioctl number just ignore size for this UI_SET_PHYS
> > > > ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
> > > > added in: https://git.kernel.org/tip/tip/c/7c7da40
> > > >
> > > > And we would not have to deal with uinput_compat_ioctl() at all.
> > >
> > > Below is example how change for removing UI_SET_PHYS_COMPAT may look
> > > like. As header file is not changed and UI_SET_PHYS accepts any size
> > > argument, it therefore accept also 32bit and 64bit integer. So no
> > > existing 32bit applications which use UI_SET_PHYS on 64bit kernel would
> > > not be broken...
> > >
> > > Is not this better change then introducing a new UI_SET_PHYS_STR ioctl
> > > number? Because introduction of new IOCTL number has one big
> > > disadvantage: Userspace applications needs to support fallback to old
> > > number as older versions of kernels would be in use for a long time. And
> > > because kernel does not have to remove old IOCTL number for backward
> > > compatibility there is basically no need for userspace application to
> > > user new UI_SET_PHYS_STR IOCTL number...
> >
> > Hello! I would like to remind this discussion as problem around a new
> > UI_SET_UNIQ ioctl is not solved yet and uniq property is really useful
> > for e.g. bluetooth (uinput) devices.
> >
> > Dmitry, when you have a time, could you please look at this discussion
> > and decide how to go ahead?
> 
> Have we decided not to move further with these changes? I actually
> have a bug in BlueZ related to it since right now we are inconsistent
> with respect to how we handle uhid vs uinput:
> 
> https://github.com/bluez/bluez/issues/352
> 
> > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > > index fd253781b..b645210d5 100644
> > > --- a/drivers/input/misc/uinput.c
> > > +++ b/drivers/input/misc/uinput.c
> > > @@ -915,22 +915,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> > >               retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
> > >               goto out;
> > >
> > > -     case UI_SET_PHYS:
> > > -             if (udev->state == UIST_CREATED) {
> > > -                     retval = -EINVAL;
> > > -                     goto out;
> > > -             }
> > > -
> > > -             phys = strndup_user(p, 1024);
> > > -             if (IS_ERR(phys)) {
> > > -                     retval = PTR_ERR(phys);
> > > -                     goto out;
> > > -             }
> > > -
> > > -             kfree(udev->dev->phys);
> > > -             udev->dev->phys = phys;
> > > -             goto out;
> > > -
> > >       case UI_BEGIN_FF_UPLOAD:
> > >               retval = uinput_ff_upload_from_user(p, &ff_up);
> > >               if (retval)
> > > @@ -1023,6 +1007,22 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> > >       case UI_ABS_SETUP & ~IOCSIZE_MASK:
> > >               retval = uinput_abs_setup(udev, p, size);
> > >               goto out;
> > > +
> > > +     case UI_SET_PHYS & ~IOCSIZE_MASK:
> > > +             if (udev->state == UIST_CREATED) {
> > > +                     retval = -EINVAL;
> > > +                     goto out;
> > > +             }
> > > +
> > > +             phys = strndup_user(p, 1024);
> > > +             if (IS_ERR(phys)) {
> > > +                     retval = PTR_ERR(phys);
> > > +                     goto out;
> > > +             }
> > > +
> > > +             kfree(udev->dev->phys);
> > > +             udev->dev->phys = phys;
> > > +             goto out;
> > >       }
> > >
> > >       retval = -EINVAL;
> > > @@ -1042,8 +1042,6 @@ static long uinput_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > >   * These IOCTLs change their size and thus their numbers between
> > >   * 32 and 64 bits.
> > >   */
> > > -#define UI_SET_PHYS_COMPAT           \
> > > -     _IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
> > >  #define UI_BEGIN_FF_UPLOAD_COMPAT    \
> > >       _IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload_compat)
> > >  #define UI_END_FF_UPLOAD_COMPAT              \
> > > @@ -1053,9 +1051,6 @@ static long uinput_compat_ioctl(struct file *file,
> > >                               unsigned int cmd, unsigned long arg)
> > >  {
> > >       switch (cmd) {
> > > -     case UI_SET_PHYS_COMPAT:
> > > -             cmd = UI_SET_PHYS;
> > > -             break;
> > >       case UI_BEGIN_FF_UPLOAD_COMPAT:
> > >               cmd = UI_BEGIN_FF_UPLOAD;
> > >               break;
> > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > index c9e677e3a..6bda2a142 100644
> > > --- a/include/uapi/linux/uinput.h
> > > +++ b/include/uapi/linux/uinput.h
> > > @@ -142,6 +142,8 @@ struct uinput_abs_setup {
> > >  #define UI_SET_LEDBIT                _IOW(UINPUT_IOCTL_BASE, 105, int)
> > >  #define UI_SET_SNDBIT                _IOW(UINPUT_IOCTL_BASE, 106, int)
> > >  #define UI_SET_FFBIT         _IOW(UINPUT_IOCTL_BASE, 107, int)
> > > +/* Argument is nul-term string and for backward compatibility is there
> > > + * specified char*, but size argument (char *) is ignored by this ioctl */
> > >  #define UI_SET_PHYS          _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > >  #define UI_SET_SWBIT         _IOW(UINPUT_IOCTL_BASE, 109, int)
> > >  #define UI_SET_PROPBIT               _IOW(UINPUT_IOCTL_BASE, 110, int)
> > >
> > >
> >
> > --
> > Pali Rohár
> > pali.rohar@gmail.com
> 
> 
> 
> -- 
> Luiz Augusto von Dentz

Hate to dig a post up from the dead, but I have a use case for this
ioctl just like the bluez team and would like to see if I can help
push it across the finish line.

Unlike this patch series mimicking what is done with the UI_SET_PHYS
ioctl, I'd like to simply have a fixed size of 64 characters allowed.
I'm choosing 64 because that's the same size as the uniq value in the
hid_device struct (specifically it's set as `char uniq[64]`).

Would such a specific limit be acceptable, and if so it shouldn't have
all the messy compatible bits like the proposed method here, would it?

Thank you,
Chris

Re: [PATCH] Input: uinput - Add UI_SET_UNIQ ioctl handler

[Pali Rohár]

On Tuesday 06 February 2024 11:17:12 Chris Morgan wrote:
> On Mon, Jun 13, 2022 at 02:36:18PM -0700, Luiz Augusto von Dentz wrote:
> > Hi,
> > 
> > On Sun, Mar 22, 2020 at 8:47 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > >
> > > On Wednesday 18 December 2019 12:26:59 Pali Rohár wrote:
> > > > On Wednesday 18 December 2019 12:02:24 Pali Rohár wrote:
> > > > > On Friday 06 December 2019 09:40:48 Dmitry Torokhov wrote:
> > > > > > On Fri, Dec 06, 2019 at 10:11:14AM +0100, Pali Rohár wrote:
> > > > > > > On Thursday 05 December 2019 12:03:05 Abhishek Pandit-Subedi wrote:
> > > > > > > > On Thu, Dec 5, 2019 at 2:52 AM Pali Rohár <pali.rohar@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Tuesday 03 December 2019 11:11:12 Dmitry Torokhov wrote:
> > > > > > > > > > On Tue, Dec 03, 2019 at 06:38:21PM +0100, Pali Rohár wrote:
> > > > > > > > > > > On Tuesday 03 December 2019 00:09:47 Pali Rohár wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Dmitry!
> > > > > > > > > > >
> > > > > > > > > > > I was looking again at those _IOW defines for ioctl calls and I have
> > > > > > > > > > > another argument why not specify 'char *' in _IOW:
> > > > > > > > > > >
> > > > > > > > > > > All ioctls in _IOW() specify as a third macro argument type which is
> > > > > > > > > > > passed as pointer to the third argument for ioctl() syscall.
> > > > > > > > > > >
> > > > > > > > > > > So e.g.:
> > > > > > > > > > >
> > > > > > > > > > >   #define EVIOCSCLOCKID _IOW('E', 0xa0, int)
> > > > > > > > > > >
> > > > > > > > > > > is called from userspace as:
> > > > > > > > > > >
> > > > > > > > > > >   int val;
> > > > > > > > > > >   ioctl(fd, EVIOCSCLOCKID, &val);
> > > > > > > > > > >
> > > > > > > > > > > Or
> > > > > > > > > > >
> > > > > > > > > > >   #define EVIOCSMASK _IOW('E', 0x93, struct input_mask)
> > > > > > > > > > >
> > > > > > > > > > > is called as:
> > > > > > > > > > >
> > > > > > > > > > >   struct input_mask val;
> > > > > > > > > > >   ioctl(fd, EVIOCSMASK, &val);
> > > > > > > > > > >
> > > > > > > > > > > So basically third argument for _IOW specify size of byte buffer passed
> > > > > > > > > > > as third argument for ioctl(). In _IOW is not specified pointer to
> > > > > > > > > > > struct input_mask, but struct input_mask itself.
> > > > > > > > > > >
> > > > > > > > > > > And in case you define
> > > > > > > > > > >
> > > > > > > > > > >   #define MY_NEW_IOCTL _IOW(UINPUT_IOCTL_BASE, 200, char*)
> > > > > > > > > > >
> > > > > > > > > > > then you by above usage you should pass data as:
> > > > > > > > > > >
> > > > > > > > > > >   char *val = "DATA";
> > > > > > > > > > >   ioctl(fd, MY_NEW_IOCTL, &val);
> > > > > > > > > > >
> > > > > > > > > > > Which is not same as just:
> > > > > > > > > > >
> > > > > > > > > > >   ioctl(fd, MY_NEW_IOCTL, "DATA");
> > > > > > > > > > >
> > > > > > > > > > > As in former case you passed pointer to pointer to data and in later
> > > > > > > > > > > case you passed only pointer to data.
> > > > > > > > > > >
> > > > > > > > > > > It just mean that UI_SET_PHYS is already defined inconsistently which is
> > > > > > > > > > > also reason why compat ioctl for it was introduced.
> > > > > > > > > >
> > > > > > > > > > Yes, you are right. UI_SET_PHYS is messed up. I guess the question is
> > > > > > > > > > what to do with all of this...
> > > > > > > > > >
> > > > > > > > > > Maybe we should define
> > > > > > > > > >
> > > > > > > > > > #define UI_SET_PHYS_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 111, len)
> > > > > > > > > > #define UI_SET_UNIQ_STR(len)  _IOC(_IOC_WRITE, UINPUT_IOCTL_BASE, 112, len)
> > > > > > > > >
> > > > > > > > > I'm not sure if this is ideal. Normally in C strings are nul-termined,
> > > > > > > > > so functions/macros do not take buffer length.
> > > > > > > > Except strncpy, strndup, snprintf, etc. all expect a buffer length. At
> > > > > > >
> > > > > > > This is something different as for these functions you pass buffer and
> > > > > > > length of buffer which is used in write mode -- not for read mode.
> > > > > > >
> > > > > > > > the user to kernel boundary of ioctl, I think we should require size
> > > > > > > > of the user buffer regardless of the data type.
> > > > > > > >
> > > > > > > > > _STR therefore in names looks inconsistent.
> > > > > > > > The _STR suffix is odd (what to name UI_SET_PHYS_STR then??) but
> > > > > > > > requiring the length seems to be common across various ioctls.
> > > > > > > > * input.h requires a length when requesting the phys and uniq
> > > > > > > > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/input.h#n138)
> > > > > > > > * Same with HIDRAW when setting and getting features:
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/hidraw.h#n40,
> > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/samples/hidraw/hid-example.c#n88
> > > > > > >
> > > > > > > All these ioctls where is passed length are in opposite direction
> > > > > > > (_IOC_READ) as our PHYS and UNIQ (_IOC_WRITE).
> > > > > > >
> > > > > > > I fully agree that when you need to read something from kernel
> > > > > > > (_IOC_READ) and then writing it to userspace, you need to specify length
> > > > > > > of userspace buffer. Exactly same as with userspace functions like
> > > > > > > memcpy, snprintf, etc... as you pointed. Otherwise you get buffer
> > > > > > > overflow as callee does not know length of buffer.
> > > > > > >
> > > > > > > But here we we have there quite different problem, we need to write
> > > > > > > something to kernel from userspace (_IOC_WRITE) and we are passing
> > > > > > > nul-term string. So in this case specifying size is not required as it
> > > > > > > is implicitly specified as part of passed string.
> > > > > >
> > > > > > With the new IOCTL definitions it does not need to be a NULL-terminated
> > > > > > string. It can be a buffer of characters with given length, and kernel
> > > > > > will NULL-terminate as this it what it wants, not what the caller has to
> > > > > > give.
> > > > >
> > > > > Hi Dmitry! I was thinking more about this problem and I will propose
> > > > > alternative solution, but first with details...
> > > > >
> > > > > I think that we should use NULL terminated strings. Or better disallow
> > > > > NULL byte inside string. Reason: all userspace application expects that
> > > > > input device name would be NULL terminated which implies that in the
> > > > > middle of name cannot be NULL byte.
> > > > >
> > > > > So this must apply also for new PHYS / UNIQ ioctl API. If we want in our
> > > > > ioctl API to use buffer + size (with upper bound limit for size) instead
> > > > > of passing NULL term string (with upper bound limit for string size)
> > > > > then kernel have to add a leading NULL byte to string, plus check that
> > > > > in the buffer there is no NULL byte. I guess this a very little
> > > > > complicate code, but nothing which is problematic.
> > > > >
> > > > > And on the userspace part. Now when userspace want to pass constant
> > > > > string for device name, it just call
> > > > >
> > > > >   ioctl(fd, UI_SET_PHYS, "my name of device");
> > > > >
> > > > > After adding a new ioctl with buffer + size API, userspace would have to
> > > > > call:
> > > > >
> > > > >   ioctl(fd, UI_SET_PHYS_STR(strlen("my name of device")), "my name of device");
> > > > >
> > > > > which looks strange, so programmers would had to move device name into
> > > > > new variable:
> > > > >
> > > > >   const char *name = "my name of device";
> > > > >   ioctl(fd, UI_SET_PHYS_STR(strlen(name)), name);
> > > > >
> > > > > For me the old ioctl API looks easier to use (no need for strlen() or
> > > > > extra variable), but this is just my preference of usage -- as it is
> > > > > simpler for me. Maybe you would have different opinion...
> > > > >
> > > > > And now question: Why we have uinput_compat_ioctl()? It is there only
> > > > > because size part of IOCTL number is different on 32bit and 64bit
> > > > > systems. As we know size part of UI_SET_PHYS is wrong and does not make
> > > > > sense...
> > > > >
> > > > > Would not it be better to change size of UI_SET_PHYS to just zero and
> > > > > then when matching ioctl number just ignore size for this UI_SET_PHYS
> > > > > ioctl? Same for UI_BEGIN_FF_UPLOAD_COMPAT and UI_END_FF_UPLOAD_COMPAT
> > > > > added in: https://git.kernel.org/tip/tip/c/7c7da40
> > > > >
> > > > > And we would not have to deal with uinput_compat_ioctl() at all.
> > > >
> > > > Below is example how change for removing UI_SET_PHYS_COMPAT may look
> > > > like. As header file is not changed and UI_SET_PHYS accepts any size
> > > > argument, it therefore accept also 32bit and 64bit integer. So no
> > > > existing 32bit applications which use UI_SET_PHYS on 64bit kernel would
> > > > not be broken...
> > > >
> > > > Is not this better change then introducing a new UI_SET_PHYS_STR ioctl
> > > > number? Because introduction of new IOCTL number has one big
> > > > disadvantage: Userspace applications needs to support fallback to old
> > > > number as older versions of kernels would be in use for a long time. And
> > > > because kernel does not have to remove old IOCTL number for backward
> > > > compatibility there is basically no need for userspace application to
> > > > user new UI_SET_PHYS_STR IOCTL number...
> > >
> > > Hello! I would like to remind this discussion as problem around a new
> > > UI_SET_UNIQ ioctl is not solved yet and uniq property is really useful
> > > for e.g. bluetooth (uinput) devices.
> > >
> > > Dmitry, when you have a time, could you please look at this discussion
> > > and decide how to go ahead?
> > 
> > Have we decided not to move further with these changes? I actually
> > have a bug in BlueZ related to it since right now we are inconsistent
> > with respect to how we handle uhid vs uinput:
> > 
> > https://github.com/bluez/bluez/issues/352
> > 
> > > > diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> > > > index fd253781b..b645210d5 100644
> > > > --- a/drivers/input/misc/uinput.c
> > > > +++ b/drivers/input/misc/uinput.c
> > > > @@ -915,22 +915,6 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> > > >               retval = uinput_set_bit(arg, propbit, INPUT_PROP_MAX);
> > > >               goto out;
> > > >
> > > > -     case UI_SET_PHYS:
> > > > -             if (udev->state == UIST_CREATED) {
> > > > -                     retval = -EINVAL;
> > > > -                     goto out;
> > > > -             }
> > > > -
> > > > -             phys = strndup_user(p, 1024);
> > > > -             if (IS_ERR(phys)) {
> > > > -                     retval = PTR_ERR(phys);
> > > > -                     goto out;
> > > > -             }
> > > > -
> > > > -             kfree(udev->dev->phys);
> > > > -             udev->dev->phys = phys;
> > > > -             goto out;
> > > > -
> > > >       case UI_BEGIN_FF_UPLOAD:
> > > >               retval = uinput_ff_upload_from_user(p, &ff_up);
> > > >               if (retval)
> > > > @@ -1023,6 +1007,22 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
> > > >       case UI_ABS_SETUP & ~IOCSIZE_MASK:
> > > >               retval = uinput_abs_setup(udev, p, size);
> > > >               goto out;
> > > > +
> > > > +     case UI_SET_PHYS & ~IOCSIZE_MASK:
> > > > +             if (udev->state == UIST_CREATED) {
> > > > +                     retval = -EINVAL;
> > > > +                     goto out;
> > > > +             }
> > > > +
> > > > +             phys = strndup_user(p, 1024);
> > > > +             if (IS_ERR(phys)) {
> > > > +                     retval = PTR_ERR(phys);
> > > > +                     goto out;
> > > > +             }
> > > > +
> > > > +             kfree(udev->dev->phys);
> > > > +             udev->dev->phys = phys;
> > > > +             goto out;
> > > >       }
> > > >
> > > >       retval = -EINVAL;
> > > > @@ -1042,8 +1042,6 @@ static long uinput_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
> > > >   * These IOCTLs change their size and thus their numbers between
> > > >   * 32 and 64 bits.
> > > >   */
> > > > -#define UI_SET_PHYS_COMPAT           \
> > > > -     _IOW(UINPUT_IOCTL_BASE, 108, compat_uptr_t)
> > > >  #define UI_BEGIN_FF_UPLOAD_COMPAT    \
> > > >       _IOWR(UINPUT_IOCTL_BASE, 200, struct uinput_ff_upload_compat)
> > > >  #define UI_END_FF_UPLOAD_COMPAT              \
> > > > @@ -1053,9 +1051,6 @@ static long uinput_compat_ioctl(struct file *file,
> > > >                               unsigned int cmd, unsigned long arg)
> > > >  {
> > > >       switch (cmd) {
> > > > -     case UI_SET_PHYS_COMPAT:
> > > > -             cmd = UI_SET_PHYS;
> > > > -             break;
> > > >       case UI_BEGIN_FF_UPLOAD_COMPAT:
> > > >               cmd = UI_BEGIN_FF_UPLOAD;
> > > >               break;
> > > > diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> > > > index c9e677e3a..6bda2a142 100644
> > > > --- a/include/uapi/linux/uinput.h
> > > > +++ b/include/uapi/linux/uinput.h
> > > > @@ -142,6 +142,8 @@ struct uinput_abs_setup {
> > > >  #define UI_SET_LEDBIT                _IOW(UINPUT_IOCTL_BASE, 105, int)
> > > >  #define UI_SET_SNDBIT                _IOW(UINPUT_IOCTL_BASE, 106, int)
> > > >  #define UI_SET_FFBIT         _IOW(UINPUT_IOCTL_BASE, 107, int)
> > > > +/* Argument is nul-term string and for backward compatibility is there
> > > > + * specified char*, but size argument (char *) is ignored by this ioctl */
> > > >  #define UI_SET_PHYS          _IOW(UINPUT_IOCTL_BASE, 108, char*)
> > > >  #define UI_SET_SWBIT         _IOW(UINPUT_IOCTL_BASE, 109, int)
> > > >  #define UI_SET_PROPBIT               _IOW(UINPUT_IOCTL_BASE, 110, int)
> > > >
> > > >
> > >
> > > --
> > > Pali Rohár
> > > pali.rohar@gmail.com
> > 
> > 
> > 
> > -- 
> > Luiz Augusto von Dentz
> 
> Hate to dig a post up from the dead, but I have a use case for this
> ioctl just like the bluez team and would like to see if I can help
> push it across the finish line.
> 
> Unlike this patch series mimicking what is done with the UI_SET_PHYS
> ioctl, I'd like to simply have a fixed size of 64 characters allowed.
> I'm choosing 64 because that's the same size as the uniq value in the
> hid_device struct (specifically it's set as `char uniq[64]`).
> 
> Would such a specific limit be acceptable, and if so it shouldn't have
> all the messy compatible bits like the proposed method here, would it?
> 
> Thank you,
> Chris

In email <20191218112659.crabhqkbcnxd6fo6@pali> (quoted above) I sent a
patch proposal for this issue. But nobody reacted for it yet.