Messages in this thread | | | Date | Thu, 31 Aug 2006 00:40:49 -0700 | From | Andrew Morton <> | Subject | Re: [RFC] Simple userspace interface for PCI drivers |
| |
On Tue, 29 Aug 2006 23:23:38 -0700 Greg KH <greg@kroah.com> wrote:
> +static ssize_t store_sig_pid(struct device *dev, struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + iio_dummy_signal.pid = simple_strtol(buf, NULL, 10); > + if (iio_dummy_signal.pid == 0) { > + if (iio_dummy_signal.it_process) { > + put_task_struct(iio_dummy_signal.it_process); > + iio_dummy_signal.it_process = NULL; > + } > + > + iio_dummy_signal.pid = 0; > + return count; > + } > + > + if (iio_dummy_signal.pid == 1) > + goto out; > + > + iio_dummy_signal.it_process = find_task_by_pid(iio_dummy_signal.pid); > + if (iio_dummy_signal.it_process) { > + get_task_struct(iio_dummy_signal.it_process); > + iio_dummy_signal.it_sigev_notify = SIGEV_SIGNAL; > + iio_dummy_signal.it_sigev_signo = SIGALRM; > + iio_dummy_signal.it_sigev_value.sival_int = 0; > + > + return count; > + } > +out: > + iio_dummy_signal.pid = 0; > + return -EINVAL; > +}
This is racy: find_task_by_pid() needs tasklist_lock or rcu_read_lock().
It doesn't work as a module due to missing __put_task_struct.
It is also rather nasty. Why go shoving some random pid into a sysfs file, then hang onto a ref on a task_struct for some process which exitted last week? It would be cleaner and more idiomatic to require that the controlling process hold an fd open against the instance so that resources can be managed correctly. Maybe use SIGIO too, so the driver doesn't need to know about pids and task_structs and things. Which all maps better onto ioctls than sysfs (ties self to stake)
<looks>
iio_dev.c seems to already be doing this. Why does iio_dummy.c exist? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |