On Fri, Feb 02, 2007 at 08:31:57AM +0100, Duncan Sands wrote:
> > I believe, barriers not needed, not now.
> > This scheme relies on the fact that remove_proc_entry() will be the only
> > place that will clear ->proc_fops and, once cleared, ->proc_fops will
> > never be resurrected. Clearing of ->proc_fops will eventually propagate
> > to CPU doing first check, thus preveting refcount bumps from this CPU.
> > What can be missed is some "rogue" readers or writers¹. Big deal.
>
> I don't understand you. Without memory barriers, remove_proc_entry will
> most of the time, but not all of the time, wait for all readers and writers
> to finish before exiting. Since the whole point of your patch was to ensure
> that all readers and writers finish before remove_proc_entry exits, I don't
> understand why you don't just put the memory barriers in and make it correct.
Gee, thanks. I sat and wrote code side-by-side, and it looks like, even barriers
won't fix anything, because they don't affect other CPUs. There will be
new patch soon.
->proc_fops is valid ->proc_fops is valid
->pde_users is 0 ->pde_users is 0
------------------------------------------------------------
if (!pde->proc_fops)
goto out;
->proc_fops = NULL;
if (atomic_read(->pde_users) > 0)
goto again;
|
| atomic_inc(->pde_users);
|
|
|
V
> Also, I do consider it a big deal:
>
> > ¹ Sigh, modules should do removals of proc entries first. And I should
> > check for that.
>
> Modules should of course call remove_proc_entry before exiting. However
> right now, even with your patch, a read or write method can still be
> running when remove_proc_entry returns [1], so could still be running when
> the module is removed (if they sleep; I guess this applies mostly to
> write methods). This is very bad - why not put in memory barriers and
> fix it? Also, plenty of proc read and write methods access private data
> that is allocated before calling create_proc_entry and freed after calling
> remove_proc_entry. If a read or write method is still running after
> remove_proc_entry returns, then it can access freed memory - very bad.
> [1] proc_get_inode does a try_module_get, so it is possible that module
> unloading is not a problem - not sure.
Modules forget to set ->owner sometimes. Also, it's still racy, because
of the typical
pde = create_proc_entry();
/*
*
* ->owner is NULL here, effectively, PDE without ->owner.
*
*/
if (pde)
pde->owner = THIS_MODULE;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
