Linux-Bluetooth Archive on lore.kernel.org
 help / color / Atom feed
From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
To: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
Cc: "Pali Rohár" <pali.rohar@gmail.com>,
	linux-input@vger.kernel.org, linux-bluetooth@vger.kernel.org,
	"Luiz Augusto von Dentz" <luiz.dentz@gmail.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	linux-kernel@vger.kernel.org,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Logan Gunthorpe" <logang@deltatee.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"Andrey Smirnov" <andrew.smirnov@gmail.com>,
	"Kirill Smelkov" <kirr@nexedi.com>
Subject: Re: [PATCH v3] Input: uinput - Add UI_SET_PHYS_STR and UI_SET_UNIQ_STR
Date: Wed, 4 Dec 2019 17:39:04 -0800
Message-ID: <20191205013904.GP50317@dtor-ws> (raw)
In-Reply-To: <20191204135434.v3.1.Ib53f70556ffe94d9a1903632ee9b0dc929f94557@changeid>

On Wed, Dec 04, 2019 at 01:55:35PM -0800, Abhishek Pandit-Subedi wrote:
> The ioctl definition for UI_SET_PHYS is ambiguous because it is defined
> with size = sizeof(char*) but is expected to be given a variable length
> string. Add a deprecation notice for UI_SET_PHYS and provide
> UI_SET_PHYS_STR(len) which expects a size from the user.
> 
> Also 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,
> 
> Here is an updated patch that refactors the ioctl handlers (properly
> allowing the size to be set from userspace). When calling the new
> ioctls, the call signature will look like this:
> 
> ```
> ioctl(fd, UI_SET_PHYS_STR(18), "00:11:22:33:44:55");
> ```
> 
> I've tested this on a Chromebook running kernel v4.19 with a sample
> program compiled for both 32-bit (i.e. gcc -m32 test.c) and 64-bit.
> 
> The final uinput device looks like this:
> 
> ```
> udevadm info -a -p /devices/virtual/input/input18
> 
> 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/input18':
>     KERNEL=="input18"
>     SUBSYSTEM=="input"
>     DRIVER==""
>     ATTR{inhibited}=="0"
>     ATTR{name}=="Test"
>     ATTR{phys}=="00:00:00:33:44:55"
>     ATTR{properties}=="0"
>     ATTR{uniq}=="00:11:22:00:00:00"
> 
> ```
> 
> 
> Changes in v3:
> - Added UI_SET_PHYS_STR(len) and UI_SET_UNIQ_STR(len) and added
>   deprecation notice for UI_SET_PHYS
> 
> Changes in v2:
> - Added compat handling for UI_SET_UNIQ
> 
>  drivers/input/misc/uinput.c | 41 ++++++++++++++++++++++++++++++++++++-
>  include/uapi/linux/uinput.h |  5 +++++
>  2 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
> index 84051f20b18a..27a750896c71 100644
> --- a/drivers/input/misc/uinput.c
> +++ b/drivers/input/misc/uinput.c
> @@ -20,6 +20,7 @@
>   */
>  #include <uapi/linux/uinput.h>
>  #include <linux/poll.h>
> +#include <linux/printk.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/module.h>
> @@ -280,7 +281,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 +290,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 +299,7 @@ static void uinput_destroy_device(struct uinput_device *udev)
>  		}
>  		kfree(name);
>  		kfree(phys);
> +		kfree(uniq);
>  		udev->dev = NULL;
>  	}
>  }
> @@ -840,6 +843,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;
>  
> @@ -916,6 +920,8 @@ static long uinput_ioctl_handler(struct file *file, unsigned int cmd,
>  		goto out;
>  
>  	case UI_SET_PHYS:
> +		pr_warn_once("uinput: UI_SET_PHYS is deprecated. Use UI_SET_PHYS_STR");
> +
>  		if (udev->state == UIST_CREATED) {
>  			retval = -EINVAL;
>  			goto out;
> @@ -1023,6 +1029,39 @@ 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_STR(0):
> +		if (udev->state == UIST_CREATED) {
> +			retval = -EINVAL;
> +			goto out;
> +		}
> +
> +		phys = strndup_user(p, size);
> +		if (IS_ERR(phys)) {
> +			retval = PTR_ERR(phys);
> +			goto out;
> +		}
> +
> +		kfree(udev->dev->phys);
> +		udev->dev->phys = phys;

Could we maybe refactor this into uinput_get_user_str(udev,
&udev->dev->phys, p, size) ?

> +		goto out;
> +
> +	case UI_SET_UNIQ_STR(0):
> +		if (udev->state == UIST_CREATED) {
> +			retval = -EINVAL;
> +			goto out;
> +		}
> +
> +		uniq = strndup_user(p, size);
> +		if (IS_ERR(uniq)) {
> +			retval = PTR_ERR(uniq);
> +			goto out;
> +		}
> +
> +		kfree(udev->dev->uniq);
> +		udev->dev->uniq = uniq;
> +		goto out;
> +
>  	}
>  
>  	retval = -EINVAL;
> diff --git a/include/uapi/linux/uinput.h b/include/uapi/linux/uinput.h
> index c9e677e3af1d..84d4fa142830 100644
> --- a/include/uapi/linux/uinput.h
> +++ b/include/uapi/linux/uinput.h
> @@ -142,9 +142,14 @@ 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)
> +
> +/* DEPRECATED: Data size is ambiguous. Use UI_SET_PHYS_STR instead. */
>  #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_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)
>  
>  #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.393.g34dc348eaf-goog
> 

-- 
Dmitry

      reply index

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-04 21:55 Abhishek Pandit-Subedi
2019-12-05  1:39 ` Dmitry Torokhov [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191205013904.GP50317@dtor-ws \
    --to=dmitry.torokhov@gmail.com \
    --cc=abhishekpandit@chromium.org \
    --cc=andrew.smirnov@gmail.com \
    --cc=bhelgaas@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=kirr@nexedi.com \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-input@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=logang@deltatee.com \
    --cc=luiz.dentz@gmail.com \
    --cc=pali.rohar@gmail.com \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Bluetooth Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-bluetooth/0 linux-bluetooth/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-bluetooth linux-bluetooth/ https://lore.kernel.org/linux-bluetooth \
		linux-bluetooth@vger.kernel.org
	public-inbox-index linux-bluetooth

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-bluetooth


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git