On Fri, Apr 2, 2021 at 11:59 PM Zheyu Ma <zheyum...@gmail.com> wrote:
>
> case NOSY_IOC_START:
> + list_for_each_entry(tmp, &client->lynx->client_list, link)
> + if (tmp == client)
> + return -EINVAL;
I don't think this is safe.
You are doing this list traversal outside the lock that protects it,
which it taken a line later:
> spin_lock_irq(client_list_lock);
> list_add_tail(&client->link, &client->lynx->client_list);
> spin_unlock_irq(client_list_lock);
so the locking is wrong.
However, I think that the proper fix is not just to move the code
inside the locked region (which makes the error handling a bit more
complex than just a return, of course), but to actually instead of
traversing the list, just look if the "client->link" list is empty.
That's what some other parts of that driver already do (ie
nosy_poll()), so I think that ->link field is already always
initialized properly (and it looks like all the list removal is using
"list_del_init()" to initialize it after removing it from a list.
So I think the patch should be something along the lines of
--- a/drivers/firewire/nosy.c
+++ b/drivers/firewire/nosy.c
@@ -346,6 +346,7 @@ nosy_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
struct client *client = file->private_data;
spinlock_t *client_list_lock = &client->lynx->client_list_lock;
struct nosy_stats stats;
+ int ret;
switch (cmd) {
case NOSY_IOC_GET_STATS:
@@ -360,11 +361,15 @@ nosy_ioctl(struct file *file,
return 0;
case NOSY_IOC_START:
+ ret = -EBUSY;
spin_lock_irq(client_list_lock);
- list_add_tail(&client->link, &client->lynx->client_list);
+ if (list_empty(&client->link)) {
+ list_add_tail(&client->link,
&client->lynx->client_list);
+ ret = 0;
+ }
spin_unlock_irq(client_list_lock);
- return 0;
+ return ret;
case NOSY_IOC_STOP:
spin_lock_irq(client_list_lock);
instead. The above is obviously white-space damaged (on purpose - I
don't want to take credit for this patch, I didn't find the problem,
and I have not tested the above in any shape or form).
Zheyu Ma, does something like that work for you?
Comments? Anybody else?
Linus
