Moving fasync_struct into struct file?

Moving fasync_struct into struct file?

[Rusty Russell]

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.

Re: Moving fasync_struct into struct file?

[Alan Cox]

> 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

Re: Moving fasync_struct into struct file?

[kuznet]

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

Re: Moving fasync_struct into struct file?

[Alan Cox]

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

Re: Moving fasync_struct into struct file?

[kuznet]

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

Re: Moving fasync_struct into struct file?

[Bob Dunlop]

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

Re: Moving fasync_struct into struct file?

[kuznet]

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

Re: Moving fasync_struct into struct file?

[Andi Kleen]

Rusty Russell <rusty@rustcorp.com.au> 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