develoment workflow: how to avoid duplicate work ?
Hi, I've started to take a look at TODO entries in the staging drivers subtree and found some issues in pi433 I'd like to work on. Before starting to prepare my patch, I tried to check the LKML and the bug tracker to make sure nobody was working on the same issues as me (this driver seems to be pretty actively developed), but couldn't find anything helpful. No mailing list, nobody coordinating, only single patches without relationship. Did I miss something ? Is there a specific place where I can coordinate with the rest of the kernel dev community and make people aware I'm working on this particular issue ? (apart from the bug tracker, which doesn't seem to be very active when it comes to the staging subtree) Thanks ! Regards, Hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Mon, 28 May 2018 23:07:06 -0400, Hugo Lefeuvre said:
Did I miss something ? Is there a specific place where I can coordinate with the rest of the kernel dev community and make people aware I'm working on this particular issue ? (apart from the bug tracker, which doesn't seem to be very active when it comes to the staging subtree)
The canonical place to check would be the git tree for drivers/staging: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/ In general, if there's other people doing active development, they'll show up in 'git log -- drivers/staging/pi433/' And there's only one person doing a lot of patching - and most of those are coding style - though he did do a locking fix in April. What particular issue are you looking at?
Hi Valdis,
The canonical place to check would be the git tree for drivers/staging:
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/
In general, if there's other people doing active development, they'll show up in 'git log -- drivers/staging/pi433/'
And there's only one person doing a lot of patching - and most of those are coding style - though he did do a locking fix in April.
What particular issue are you looking at?
For example the TODO entry at line 876: If pi433_release() is called while pi433_ioctl() is executing between lines 879 and 880, we might perform a NULL pointer dereference, right ? Regards, Hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Tue, 29 May 2018 22:56:57 -0400, Hugo Lefeuvre said:
For example the TODO entry at line 876: If pi433_release() is called while pi433_ioctl() is executing between lines 879 and 880, we might perform a NULL pointer dereference, right ?
Yes, no, maybe. That's what kernel locks are for. Is that data protected against concurrent access by a lock of some sort?
For example the TODO entry at line 876: If pi433_release() is called while pi433_ioctl() is executing between lines 879 and 880, we might perform a NULL pointer dereference, right ?
Yes, no, maybe. That's what kernel locks are for. Is that data protected against concurrent access by a lock of some sort?
No, I don't think so. The release function doesn't ask for any kind of lock before freeing that data, nor does the ioctl function. Also, this ioctl function is unlocked_ioctl, so AFAIK it should be self responsible for locking/synchronization stuff (most docs I've read are getting pretty old now, from the 2.6 times where the BKL was still something 'common' and lots of drivers were still using ioctl(), but I don't think it's the case anymore). So, if pi433_release() and pi433_ioctl() can be concurrently executed then this issue might happen. I'll submit a patch. Thanks ! Cheers, Hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Wed, May 30, 2018 at 09:44:32PM -0400, Hugo Lefeuvre wrote:
So, if pi433_release() and pi433_ioctl() can be concurrently executed then this issue might happen.
But isn't release() called when nothing is referencing the file anymore, so there should be no other file operation running concurrently? -- Valentin
So, if pi433_release() and pi433_ioctl() can be concurrently executed then this issue might happen.
But isn't release() called when nothing is referencing the file anymore, so there should be no other file operation running concurrently?
The vfs documentation states: release() is "called when the last reference to an open file is closed". Let's say we have a program with threads T1 and T2. - T1 calls ioctl on a file descriptor FD. - (on another processor) T2 closes FD. Since the last reference to FD was closed by T2, release is called. But while release is being called, the ioctl call from T1 may still be running, right ? -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Sun, Jun 03, 2018 at 06:25:56PM -0400, Hugo Lefeuvre wrote:
The vfs documentation states: release() is "called when the last reference to an open file is closed".
Let's say we have a program with threads T1 and T2.
- T1 calls ioctl on a file descriptor FD. - (on another processor) T2 closes FD.
Since the last reference to FD was closed by T2, release is called. But while release is being called, the ioctl call from T1 may still be running, right ?
Indeed, I did a quick test and close can return in userspace before ioctl has finished. But in kernel space release does not even start until ioctl has finished. So it seems there exists some mechanism to delay release until all operations on a file descriptor have finished? View from userspace: 1528082878 opened file 3 1528082878 ioctl start 3 1528082879 close start 3 1528082879 close return 0 1528082883 ioctl return 0 View from kernelspace: [215422.216734] pi433 pi433.0: ioctl: sleep [215427.296761] pi433 pi433.0: ioctl: start [215427.296772] pi433 pi433.0: ioctl: rdtx [215427.296783] pi433 pi433.0: ioctl: rdtx done [215427.296792] pi433 pi433.0: ioctl: return 0 [215427.296807] pi433 pi433.0: release: start [215427.296816] pi433 pi433.0: release: return -- Valentin
On Mon, 04 Jun 2018 05:33:03 +0200, Valentin Vidic said:
On Sun, Jun 03, 2018 at 06:25:56PM -0400, Hugo Lefeuvre wrote:
The vfs documentation states: release() is "called when the last reference to an open file is closed".
Let's say we have a program with threads T1 and T2.
- T1 calls ioctl on a file descriptor FD. - (on another processor) T2 closes FD.
Since the last reference to FD was closed by T2, release is called.
That's subtly wrong. T2 releases its reference to the file descriptor.
But while release is being called, the ioctl call from T1 may still be running, right ?
Remember that ioctl needs an open FD as well - so the ioctl() grabs its own reference, and then *that* reference to the file descriptor stays in place at least until the ioctl() return. At *that* point, the reference count goes to zero and the file is actually closed.
The vfs documentation states: release() is "called when the last reference to an open file is closed".
Let's say we have a program with threads T1 and T2.
- T1 calls ioctl on a file descriptor FD. - (on another processor) T2 closes FD.
Since the last reference to FD was closed by T2, release is called.
That's subtly wrong. T2 releases its reference to the file descriptor.
But while release is being called, the ioctl call from T1 may still be running, right ?
Remember that ioctl needs an open FD as well - so the ioctl() grabs its own reference, and then *that* reference to the file descriptor stays in place at least until the ioctl() return. At *that* point, the reference count goes to zero and the file is actually closed.
Well, my assumption was that T1 and T2 would share the exact same file descriptor. For example, a main thread T0 would call open() to get the file descriptor, and then spawn T1 and T2 which would both use this common FD. Let's say: - main thread T0 calls open() and gets FD 3 - T0 spawns T1 and T2 - T1 calls ioctl(3, ...) or read(3, ...)/write(3, ...) - (on another processor) T2 calls close(3) Do you mean that the ioctl/read/write call increments the reference count in this case ? It would mean that these syscalls aren't really using passed FD but instead create duplicates to make sure the open file description won't be freed during their execution, right ? Cheers, hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Mon, 04 Jun 2018 18:31:31 -0400, Hugo Lefeuvre said:
Do you mean that the ioctl/read/write call increments the reference count in this case ? It would mean that these syscalls aren't really using passed FD but instead create duplicates to make sure the open file description won't be freed during their execution, right ?
One file descriptor is passed around, and each syscall or other code that needs to protect it from evaporating out from under it takes a reference. Think of it as "How many of you are still using it? 3? OK.. I won't clean up yet. Oh, it's down to zero? OK, it's clean up time" Another example of the same sort of thing can be seen in file systems, where one or more file descriptors can be opened on a given file, and the file is then unlinked - but the inode and the allocated space doesn't actually get freed until all the open descriptors (each of which increments the ref count) are closed and the refcount actually reaches zero. That's what's going on when you run 'lsof' and see files listed as '(deleted)', or when you *think* you've cleaned up the logs, but /var is still sitting at 98%....
Do you mean that the ioctl/read/write call increments the reference count in this case ? It would mean that these syscalls aren't really using passed FD but instead create duplicates to make sure the open file description won't be freed during their execution, right ?
One file descriptor is passed around, and each syscall or other code that needs to protect it from evaporating out from under it takes a reference.
Thanks. I think I'll have to read the source code to fully understand what happens. Do you know what piece of code handles this reference duplication ? As a conclusion we can assume that ioctl and release never run concurrently, and as such the lock introduced in my patch is useless. Concerning the TODO at line 876, I think I've misunderstood it. I'll think a bit more about it and come back with an updated patch later. Thanks ! Cheers, Hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Tue, 05 Jun 2018 10:20:16 -0400, Hugo Lefeuvre said:
Thanks. I think I'll have to read the source code to fully understand what happens. Do you know what piece of code handles this reference duplication ?
It's not duplication, it's increment/decrement of a counter. Look for functions with 'get' and 'put' in their names.
As a conclusion we can assume that ioctl and release never run concurrently, and as such the lock introduced in my patch is useless.
Well, they *shouldn't* do so. What the code actually does, however....
Concerning the TODO at line 876, I think I've misunderstood it. I'll think a bit more about it and come back with an updated patch later.
I haven't looked at the code - there's an outside chance that the driver isn't doing reference counting correctly.
On Tue, Jun 05, 2018 at 10:33:21AM -0400, valdis.kletnieks@vt.edu wrote:
It's not duplication, it's increment/decrement of a counter.
Look for functions with 'get' and 'put' in their names.
AFAICT counter is f_count in struct file, updated by fget and fput.
I haven't looked at the code - there's an outside chance that the driver isn't doing reference counting correctly.
pi433 driver does not mess with struct file too much so I guess it should be safe :) pi433_read(struct file *filp, char __user *buf, size_t size, loff_t *f_pos) instance = filp->private_data; pi433_write(struct file *filp, const char __user *buf, instance = filp->private_data; pi433_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) instance = filp->private_data; pi433_compat_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) return pi433_ioctl(filp, cmd, (unsigned long)compat_ptr(arg)); static int pi433_open(struct inode *inode, struct file *filp) filp->private_data = instance; nonseekable_open(inode, filp); static int pi433_release(struct inode *inode, struct file *filp) instance = filp->private_data; filp->private_data = NULL; -- Valentin
It's not duplication, it's increment/decrement of a counter.
Look for functions with 'get' and 'put' in their names.
AFAICT counter is f_count in struct file, updated by fget and fput.
Yes, this is the counter I was speaking about. But to me the only way to increment/decrement this counter was either to duplicate an existing reference using dup/dup2, or to fork the process. This was wrong, and I'm glad I learned that. FTR: After taking a look at the kernel code, the ioctl syscall is implemented in ksys_ioctl[0], which calls fdget/fdput to get the struct fd, incrementing and decrementing the count. Thanks your help !
I haven't looked at the code - there's an outside chance that the driver isn't doing reference counting correctly.
pi433 driver does not mess with struct file too much so I guess it should be safe :)
I'll take a closer look at the TODO and come back later with a patch. If there's nothing to do I'll remove it, otherwise I'll fix it. Cheers, Hugo [0] https://elixir.bootlin.com/linux/latest/source/fs/ioctl.c#L692 -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
On Mon, May 28, 2018 at 11:07:06PM -0400, Hugo Lefeuvre wrote:
Hi,
I've started to take a look at TODO entries in the staging drivers subtree and found some issues in pi433 I'd like to work on. Before starting to prepare my patch, I tried to check the LKML and the bug tracker to make sure nobody was working on the same issues as me (this driver seems to be pretty actively developed), but couldn't find anything helpful. No mailing list, nobody coordinating, only single patches without relationship.
Did I miss something ? Is there a specific place where I can coordinate with the rest of the kernel dev community and make people aware I'm working on this particular issue ? (apart from the bug tracker, which doesn't seem to be very active when it comes to the staging subtree)
No need to coordinate anything, just start sending patches against the latest development tree (linux-next, or the staging.git staging-next branch), and all will be fine. If there are conflicts, the maintainer will let you know, but they are usually quite rare. good luck! greg k-h
Hi Greg,
No need to coordinate anything, just start sending patches against the latest development tree (linux-next, or the staging.git staging-next branch), and all will be fine.
If there are conflicts, the maintainer will let you know, but they are usually quite rare.
Then I will simply submit my patch and let the maintainer check for conflicts. Thanks for your answer. Cheers, Hugo -- Hugo Lefeuvre (hle) | www.owl.eu.com 4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
participants (4)
-
Greg KH -
Hugo Lefeuvre -
valdis.kletnieks＠vt.edu -
Valentin Vidic