Re: [patch 03/11] input: new force feedback interface

[Andrew Morton <akpm@osdl.org>]

Anssi Hannula <anssi@mandriva.org> wrote:
>
>  >>+int input_ff_erase(struct input_dev *dev, int id)
>  >>+{
>  >>+	struct ff_device *ff;
>  >>+	unsigned long flags = 0;
>  >>+	int ret;
>  >>+	if (!test_bit(EV_FF, dev->evbit))
>  >>+		return -EINVAL;
>  >>+	mutex_lock(&dev->ff_lock);
>  >>+	ff = dev->ff;
>  >>+	if (!ff) {
>  >>+		mutex_unlock(&dev->ff_lock);
>  >>+		return -ENODEV;
>  >>+	}
>  >>+	spin_ff_cond_lock(ff, flags);
>  >>+	ret = _input_ff_erase(dev, id, current->pid == 0);
>  >>+	spin_ff_cond_unlock(ff, flags);
>  >>+
>  >>+	mutex_unlock(&dev->ff_lock);
>  >>+	return ret;
>  >>+}
>  > 
>  > 
>  > Perhaps you meant `current->uid == 0' here.  There's no way in which pid
>  > 0 will call this code.
> 
>  Right, a silly mistake.
> 
>  > What's happening here anyway?  Why does this code need to know about pids?
>  > 
>  > Checking for uid==0 woud be a fishy thing to do as well.
> 
>  User ID 0 is allowed to delete effects of other users. Pids are used to
>  keep a track of what process owns what effects. This is the same
>  behaviour as before.

Oh dear.

Whatever we do here should remain 100%-compatible with "before".  Which
rather limits our options.

>  There is a problem with this, though:
>  When a process closes any fd to this device, all pid-matching effects
>  are deleted whether the process has another fd using the device or not.
> 
>  One solution would probably be to add some handle parameter to
>  input_ff_upload() and input_ff_erase(), and then in
>  evdev_ioctl_handler() pass an id unique to this fd. Then effects would
>  be fd-specific, not pid-specific. I think the uid == 0 thing can also be
>  dropped... I don't think the root user needs ability to override user
>  effects (it can delete them anyway, just kill the user process owning
>  the effects).
> 

Generally we use file descriptors (and driver-specific state at
file.f_private) to manage things like that.  But I'd imagine that we
couldn't retain the existing semantics with any such scheme.

A pragmatic approach would be to put a big fat comment in there explaining
how it all works and leave it at that.