All of lore.kernel.org
 help / color / mirror / Atom feed
* develoment workflow: how to avoid duplicate work ?
@ 2018-05-29  3:07 Hugo Lefeuvre
  2018-05-29  3:32 ` valdis.kletnieks at vt.edu
  2018-05-29  5:50 ` Greg KH
  0 siblings, 2 replies; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-05-29  3:07 UTC (permalink / raw)
  To: kernelnewbies

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180528/90908f68/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-29  3:07 develoment workflow: how to avoid duplicate work ? Hugo Lefeuvre
@ 2018-05-29  3:32 ` valdis.kletnieks at vt.edu
  2018-05-30  2:56   ` Hugo Lefeuvre
  2018-05-29  5:50 ` Greg KH
  1 sibling, 1 reply; 19+ messages in thread
From: valdis.kletnieks at vt.edu @ 2018-05-29  3:32 UTC (permalink / raw)
  To: kernelnewbies

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180528/c1154b51/attachment-0001.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-29  3:07 develoment workflow: how to avoid duplicate work ? Hugo Lefeuvre
  2018-05-29  3:32 ` valdis.kletnieks at vt.edu
@ 2018-05-29  5:50 ` Greg KH
  2018-05-30  3:08   ` Hugo Lefeuvre
  1 sibling, 1 reply; 19+ messages in thread
From: Greg KH @ 2018-05-29  5:50 UTC (permalink / raw)
  To: kernelnewbies

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-29  3:32 ` valdis.kletnieks at vt.edu
@ 2018-05-30  2:56   ` Hugo Lefeuvre
  2018-05-30 15:03     ` valdis.kletnieks at vt.edu
  0 siblings, 1 reply; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-05-30  2:56 UTC (permalink / raw)
  To: kernelnewbies

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180529/83faea10/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-29  5:50 ` Greg KH
@ 2018-05-30  3:08   ` Hugo Lefeuvre
  0 siblings, 0 replies; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-05-30  3:08 UTC (permalink / raw)
  To: kernelnewbies

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180529/44982563/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-30  2:56   ` Hugo Lefeuvre
@ 2018-05-30 15:03     ` valdis.kletnieks at vt.edu
  2018-05-31  1:44       ` Hugo Lefeuvre
  0 siblings, 1 reply; 19+ messages in thread
From: valdis.kletnieks at vt.edu @ 2018-05-30 15:03 UTC (permalink / raw)
  To: kernelnewbies

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180530/89da2e93/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-30 15:03     ` valdis.kletnieks at vt.edu
@ 2018-05-31  1:44       ` Hugo Lefeuvre
  2018-06-03 19:23         ` Valentin Vidic
  0 siblings, 1 reply; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-05-31  1:44 UTC (permalink / raw)
  To: kernelnewbies

> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180530/532598bc/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-05-31  1:44       ` Hugo Lefeuvre
@ 2018-06-03 19:23         ` Valentin Vidic
  2018-06-03 22:25           ` Hugo Lefeuvre
  0 siblings, 1 reply; 19+ messages in thread
From: Valentin Vidic @ 2018-06-03 19:23 UTC (permalink / raw)
  To: kernelnewbies

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-03 19:23         ` Valentin Vidic
@ 2018-06-03 22:25           ` Hugo Lefeuvre
  2018-06-04  3:33             ` Valentin Vidic
  0 siblings, 1 reply; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-06-03 22:25 UTC (permalink / raw)
  To: kernelnewbies

> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180603/c78b5c5e/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-03 22:25           ` Hugo Lefeuvre
@ 2018-06-04  3:33             ` Valentin Vidic
       [not found]               ` <20180604062743.GB19954@osadl.at>
  2018-06-04 16:10               ` valdis.kletnieks at vt.edu
  0 siblings, 2 replies; 19+ messages in thread
From: Valentin Vidic @ 2018-06-04  3:33 UTC (permalink / raw)
  To: kernelnewbies

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
       [not found]               ` <20180604062743.GB19954@osadl.at>
@ 2018-06-04  9:31                 ` Valentin Vidic
  2018-06-04 12:36                   ` Hugo Lefeuvre
  0 siblings, 1 reply; 19+ messages in thread
From: Valentin Vidic @ 2018-06-04  9:31 UTC (permalink / raw)
  To: kernelnewbies

On Mon, Jun 04, 2018 at 06:27:43AM +0000, Nicholas Mc Guire wrote:
> are you using kprobes to get this information or how did you trace
> these calls ? could you explain how you did that ? I assume that
> would be of interest to others as well not only me.

Nothing fancy, just added debug print statements to the release
and ioctl methods of the pi433 driver.  The ioctl method also has
a 5s sleep so it takes some time to execute while the other thread
calls close on the same file descriptor.

The timing should be as follows:

0s thread1 and thread2 start
0s thread1 calls ioctl
1s thread2 calls close
1s thread2 close returns
5s thread1 ioctl returns

Adding a dump_stack() call to both driver methods shows that ioctl is
called directly:

[236449.534358] [<8010ffd8>] (unwind_backtrace) from [<8010c240>] (show_stack+0x20/0x24)
[236449.534378] [<8010c240>] (show_stack) from [<807840a4>] (dump_stack+0xd4/0x118)
[236449.534413] [<807840a4>] (dump_stack) from [<7f5e5348>] (pi433_ioctl+0x64/0x324 [pi433])
[236449.534457] [<7f5e5348>] (pi433_ioctl [pi433]) from [<8029dbe0>] (do_vfs_ioctl+0xac/0x7c4)
[236449.534472] [<8029dbe0>] (do_vfs_ioctl) from [<8029e33c>] (SyS_ioctl+0x44/0x6c)
[236449.534488] [<8029e33c>] (SyS_ioctl) from [<80108060>] (ret_fast_syscall+0x0/0x28)

while the release gets called from task_work_run:

[236454.624185] [<8010ffd8>] (unwind_backtrace) from [<8010c240>] (show_stack+0x20/0x24)
[236454.624204] [<8010c240>] (show_stack) from [<807840a4>] (dump_stack+0xd4/0x118)
[236454.624240] [<807840a4>] (dump_stack) from [<7f5e508c>] (pi433_release+0x48/0xc0 [pi433])
[236454.624270] [<7f5e508c>] (pi433_release [pi433]) from [<8028bab0>] (__fput+0x9c/0x1e8)
[236454.624288] [<8028bab0>] (__fput) from [<8028bc6c>] (____fput+0x18/0x1c)
[236454.624304] [<8028bc6c>] (____fput) from [<8013bbf8>] (task_work_run+0xbc/0xe0)
[236454.624322] [<8013bbf8>] (task_work_run) from [<8010b810>] (do_work_pending+0xcc/0xd0)
[236454.624340] [<8010b810>] (do_work_pending) from [<80108094>] (slow_work_pending+0xc/0x20)

-- 
Valentin

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-04  9:31                 ` Valentin Vidic
@ 2018-06-04 12:36                   ` Hugo Lefeuvre
  0 siblings, 0 replies; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-06-04 12:36 UTC (permalink / raw)
  To: kernelnewbies

> Nothing fancy, just added debug print statements to the release
> and ioctl methods of the pi433 driver.  The ioctl method also has
> a 5s sleep so it takes some time to execute while the other thread
> calls close on the same file descriptor.
> 
> The timing should be as follows:
> 
> 0s thread1 and thread2 start
> 0s thread1 calls ioctl
> 1s thread2 calls close
> 1s thread2 close returns
> 5s thread1 ioctl returns
> 
> Adding a dump_stack() call to both driver methods shows that ioctl is
> called directly:
> 
> [236449.534358] [<8010ffd8>] (unwind_backtrace) from [<8010c240>] (show_stack+0x20/0x24)
> [236449.534378] [<8010c240>] (show_stack) from [<807840a4>] (dump_stack+0xd4/0x118)
> [236449.534413] [<807840a4>] (dump_stack) from [<7f5e5348>] (pi433_ioctl+0x64/0x324 [pi433])
> [236449.534457] [<7f5e5348>] (pi433_ioctl [pi433]) from [<8029dbe0>] (do_vfs_ioctl+0xac/0x7c4)
> [236449.534472] [<8029dbe0>] (do_vfs_ioctl) from [<8029e33c>] (SyS_ioctl+0x44/0x6c)
> [236449.534488] [<8029e33c>] (SyS_ioctl) from [<80108060>] (ret_fast_syscall+0x0/0x28)
> 
> while the release gets called from task_work_run:
> 
> [236454.624185] [<8010ffd8>] (unwind_backtrace) from [<8010c240>] (show_stack+0x20/0x24)
> [236454.624204] [<8010c240>] (show_stack) from [<807840a4>] (dump_stack+0xd4/0x118)
> [236454.624240] [<807840a4>] (dump_stack) from [<7f5e508c>] (pi433_release+0x48/0xc0 [pi433])
> [236454.624270] [<7f5e508c>] (pi433_release [pi433]) from [<8028bab0>] (__fput+0x9c/0x1e8)
> [236454.624288] [<8028bab0>] (__fput) from [<8028bc6c>] (____fput+0x18/0x1c)
> [236454.624304] [<8028bc6c>] (____fput) from [<8013bbf8>] (task_work_run+0xbc/0xe0)
> [236454.624322] [<8013bbf8>] (task_work_run) from [<8010b810>] (do_work_pending+0xcc/0xd0)
> [236454.624340] [<8010b810>] (do_work_pending) from [<80108094>] (slow_work_pending+0xc/0x20)

Interesting. I don't know what's happening here, but the idea that
release() would be delayed until all operations returned (as nice
as it sounds) seems pretty odd to me.

I'll not have time to experiment on this today, but this is definitely
worth taking a look.

Cheers,
 Hugo

-- 
             Hugo Lefeuvre (hle)    |    www.owl.eu.com
4096/ 9C4F C8BF A4B0 8FC5 48EB 56B8 1962 765B B9A8 BACA
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180604/7bec1450/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-04  3:33             ` Valentin Vidic
       [not found]               ` <20180604062743.GB19954@osadl.at>
@ 2018-06-04 16:10               ` valdis.kletnieks at vt.edu
  2018-06-04 22:31                 ` Hugo Lefeuvre
  1 sibling, 1 reply; 19+ messages in thread
From: valdis.kletnieks at vt.edu @ 2018-06-04 16:10 UTC (permalink / raw)
  To: kernelnewbies

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-04 16:10               ` valdis.kletnieks at vt.edu
@ 2018-06-04 22:31                 ` Hugo Lefeuvre
  2018-06-05  5:55                   ` valdis.kletnieks at vt.edu
  0 siblings, 1 reply; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-06-04 22:31 UTC (permalink / raw)
  To: kernelnewbies

> > > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180604/88175342/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-04 22:31                 ` Hugo Lefeuvre
@ 2018-06-05  5:55                   ` valdis.kletnieks at vt.edu
  2018-06-05 14:20                     ` Hugo Lefeuvre
  0 siblings, 1 reply; 19+ messages in thread
From: valdis.kletnieks at vt.edu @ 2018-06-05  5:55 UTC (permalink / raw)
  To: kernelnewbies

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%....


-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180605/03c1aa09/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-05  5:55                   ` valdis.kletnieks at vt.edu
@ 2018-06-05 14:20                     ` Hugo Lefeuvre
  2018-06-05 14:33                       ` valdis.kletnieks at vt.edu
  0 siblings, 1 reply; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-06-05 14:20 UTC (permalink / raw)
  To: kernelnewbies

> > 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180605/1e927823/attachment-0001.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-05 14:20                     ` Hugo Lefeuvre
@ 2018-06-05 14:33                       ` valdis.kletnieks at vt.edu
  2018-06-05 17:47                         ` Valentin Vidic
  0 siblings, 1 reply; 19+ messages in thread
From: valdis.kletnieks at vt.edu @ 2018-06-05 14:33 UTC (permalink / raw)
  To: kernelnewbies

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 486 bytes
Desc: not available
URL: <http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20180605/4e8b27e6/attachment.sig>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-05 14:33                       ` valdis.kletnieks at vt.edu
@ 2018-06-05 17:47                         ` Valentin Vidic
  2018-06-06  1:48                           ` Hugo Lefeuvre
  0 siblings, 1 reply; 19+ messages in thread
From: Valentin Vidic @ 2018-06-05 17:47 UTC (permalink / raw)
  To: kernelnewbies

On Tue, Jun 05, 2018 at 10:33:21AM -0400, valdis.kletnieks at 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* develoment workflow: how to avoid duplicate work ?
  2018-06-05 17:47                         ` Valentin Vidic
@ 2018-06-06  1:48                           ` Hugo Lefeuvre
  0 siblings, 0 replies; 19+ messages in thread
From: Hugo Lefeuvre @ 2018-06-06  1:48 UTC (permalink / raw)
  To: kernelnewbies

> > 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-06-06  1:48 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29  3:07 develoment workflow: how to avoid duplicate work ? Hugo Lefeuvre
2018-05-29  3:32 ` valdis.kletnieks at vt.edu
2018-05-30  2:56   ` Hugo Lefeuvre
2018-05-30 15:03     ` valdis.kletnieks at vt.edu
2018-05-31  1:44       ` Hugo Lefeuvre
2018-06-03 19:23         ` Valentin Vidic
2018-06-03 22:25           ` Hugo Lefeuvre
2018-06-04  3:33             ` Valentin Vidic
     [not found]               ` <20180604062743.GB19954@osadl.at>
2018-06-04  9:31                 ` Valentin Vidic
2018-06-04 12:36                   ` Hugo Lefeuvre
2018-06-04 16:10               ` valdis.kletnieks at vt.edu
2018-06-04 22:31                 ` Hugo Lefeuvre
2018-06-05  5:55                   ` valdis.kletnieks at vt.edu
2018-06-05 14:20                     ` Hugo Lefeuvre
2018-06-05 14:33                       ` valdis.kletnieks at vt.edu
2018-06-05 17:47                         ` Valentin Vidic
2018-06-06  1:48                           ` Hugo Lefeuvre
2018-05-29  5:50 ` Greg KH
2018-05-30  3:08   ` Hugo Lefeuvre

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.