Hi guys,
Stephen Rothwell pointed out that if you set up SIGIO from an
fd, fork, and exit, and PIDs wrap, the new process may be clobbered by
the SIGIO. IMVHO the best way to clean this up is to check the
fasync_list in sys_close, and if pid == filp->f_owner.pid and fd ==
fasync_list->fa_fd, unregister the SIGIO.
This means we need a move the "struct fasync_struct
fasync_list" into struct file (up from all the subsystems which use
it, eg. struct socket).
See any problems with this?
Rusty.
--
Anyone who quotes me in their sig is an idiot. -- Rusty Russell.
> the SIGIO. IMVHO the best way to clean this up is to check the
> fasync_list in sys_close, and if pid == filp->f_owner.pid and fd ==
> fasync_list->fa_fd, unregister the SIGIO.
We already clean up fasync structures on close, its the drivers
responsibility to do so. If you wanted to be more strict you could do
a similar helper call in the other closing callback for each fd close.
> This means we need a move the "struct fasync_struct
> fasync_list" into struct file (up from all the subsystems which use
> it, eg. struct socket).
Any reason for not just caching the 32bit task ident field ? Using the
slightly confusingly p->parent_exec_id, which is basically a 32bit process
ident for example
Rusty Russell <[email protected]> writes:
> Hi guys,
>
> Stephen Rothwell pointed out that if you set up SIGIO from an
> fd, fork, and exit, and PIDs wrap, the new process may be clobbered by
> the SIGIO. IMVHO the best way to clean this up is to check the
> fasync_list in sys_close, and if pid == filp->f_owner.pid and fd ==
> fasync_list->fa_fd, unregister the SIGIO.
The pid/owner checking at setup time is very broken anyways. Consider a
threaded application that wants to set up SIGIO from one thread but receive
from another. It has to be root currently to do that.
Best would be to never check anything in F_SETOWN, but just save the
uid/pid/gid and always check at signal sending time.
[in addition sk->proc should go and use the fasync list, but that is a
different thing for now]
-Andi
Hi,
On Tue, Feb 19, Rusty Russell wrote:
> This means we need a move the "struct fasync_struct
> fasync_list" into struct file (up from all the subsystems which use
> it, eg. struct socket).
>
> See any problems with this?
At first I thought I would clean up the drivers a little moving common
code from the release routine. The release code is not called in the
example you gave because of the fork, correct ?
Then I realised what happens if several processes all request SIGIO
notification on different descriptors. The driver still needs to keep
a private list of all the processes registered with it. struct file
should at best contain a pointer back to the relevant structure in the
driver private list for cleanup ?
--
Bob Dunlop
Hello!
> Stephen Rothwell pointed out that if you set up SIGIO from an
> fd, fork, and exit, and PIDs wrap, the new process may be clobbered by
> the SIGIO. IMVHO the best way to clean this up is to check the
> fasync_list in sys_close, and if pid == filp->f_owner.pid and fd ==
> fasync_list->fa_fd, unregister the SIGIO.
>
> This means we need a move the "struct fasync_struct
> fasync_list" into struct file (up from all the subsystems which use
> it, eg. struct socket).
>
> See any problems with this?
I do not see.
It is a long known piece of shit in the kernel crying about repair.
I remember Al Viro planned to do something with this for 2.4,
but this has been forgotten again.
Alexey
Hello!
> We already clean up fasync structures on close,
We do not.
> responsibility to do so. If you wanted to be more strict you could do
> a similar helper call in the other closing callback for each fd close.
This is technical issue how to implement this exactly.
BTW, sed -e 's/more strict/not so buggy/' :-)
Alexey
> > We already clean up fasync structures on close,
> We do not.
Then fix your driver. All the drivers I looked at do stuff like this...
which seems to do the job nicely
static int close_pad(struct inode * inode, struct file * file)
{
lock_kernel();
fasync_pad(-1, file, 0);
if (!--active)
outb(0x30, current_params.io+2); /* switch off digitiser */
unlock_kernel();
return 0;
}
Hello!
> Then fix your driver. All the drivers I looked at do stuff like this...
> which seems to do the job nicely
But this code is even not reached in the case which Paul tells about.
It should be executed earlier, as you noticed.
Alexey