LKML Archive on
help / color / mirror / Atom feed
From: Alexey Dobriyan <>
To: Duncan Sands <>
Subject: Re: remove_proc_entry and read_proc
Date: Mon, 5 Feb 2007 14:39:32 +0300	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

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

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

  reply	other threads:[~2007-02-05 11:32 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-02-01 16:09 Alexey Dobriyan
2007-02-02  7:31 ` Duncan Sands
2007-02-05 11:39   ` Alexey Dobriyan [this message]
2007-02-05 12:05     ` Duncan Sands
  -- strict thread matches above, loose matches on Subject: below --
2007-01-31 10:54 Duncan Sands
2007-01-31 18:42 ` Alexey Dobriyan
2007-01-31 19:26   ` Duncan Sands
2007-02-01 10:15     ` Duncan Sands

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \
    --subject='Re: remove_proc_entry and read_proc' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).