All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ftp.linux.org.uk>
To: Stephen Hemminger <shemminger@osdl.org>
Cc: Andrew Morton <akpm@osdl.org>,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH] handle module ref count on sysctl tables.
Date: Wed, 21 Dec 2005 19:08:49 +0000	[thread overview]
Message-ID: <20051221190849.GM27946@ftp.linux.org.uk> (raw)
In-Reply-To: <20051221103520.258ced08@dxpl.pdx.osdl.net>

On Wed, Dec 21, 2005 at 10:35:19AM -0800, Stephen Hemminger wrote:
> Right now there is a hole in the module ref counting system because
> there is no proper ref counting for sysctl tables used by modules.
> This means that if an application is holding /proc/sys/foo open and
> module that created it is unloaded, then the application touches the
> file the kernel will oops.
> 
> This patch fixes that by maintaining source compatibility via macro.
> I am sure someone already thought of this, it just doesn't appear to
> have made it in yet.

NAK.
	a) holding the file open will *NOT* pin any module structures down.
IO in progress will, but it unregistering sysctl table will block until it's
over.  The same goes for sysctl(2) in progress.  See use_table() and
friends in kernel/sysctl.c
	b) you are not protecting any code in module; what needs protection
(and gets it) is a pile of data structures.  With lifetimes that don't have
to be related to module lifetimes.  IOW, use of reference to module is 100%
wrong here - it wouldn't fix anything.

As a general rule, when you pin something down, think what you are trying
to protect; if it's not just a bunch of function references - module is
the wrong thing to hold.

In particular, sysctl tables are dynamically created and removed in a
kernel that is not modular at all.  Which kills any hope to get a solution
based on preventing rmmod.

Solution is fairly simple:
	* put use counter into sysctl table head (i.e. object allocated by
kernel/sysctl.c)
	* bump use counter when examining table in sysctl(2) and around the
actual IO in procfs access; put reference to table into proc_dir_entry to
be able to do the latter.  Decrement when done with the table; if it had
hit zero _and_ there's unregistration waiting for completion - kick it.
	* have unregistration kill all reference to table head and if use
counter is positive - wait for completion.  Once we get it, we know that
we can safely proceed.



  parent reply	other threads:[~2005-12-21 19:08 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-12-21 18:35 [PATCH] handle module ref count on sysctl tables Stephen Hemminger
2005-12-21 18:42 ` Arjan van de Ven
2005-12-21 18:47   ` Stephen Hemminger
2005-12-21 19:08 ` Al Viro [this message]
2005-12-21 19:20   ` Al Viro
2005-12-21 19:21   ` Stephen Hemminger

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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=20051221190849.GM27946@ftp.linux.org.uk \
    --to=viro@ftp.linux.org.uk \
    --cc=akpm@osdl.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=shemminger@osdl.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.