All of lore.kernel.org
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Chen Gang <gang.chen@asianux.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>,
	Oleg Nesterov <oleg@redhat.com>,
	"Eric W. Biederman" <ebiederm@xmission.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] kernel/exit.c: call read_unlock() when failure occurs after already called read_lock() in do_wait().
Date: Sat, 5 Oct 2013 07:34:32 +0100	[thread overview]
Message-ID: <20131005063431.GU13318@ZenIV.linux.org.uk> (raw)
In-Reply-To: <524FA956.9080100@asianux.com>

On Sat, Oct 05, 2013 at 01:53:26PM +0800, Chen Gang wrote:
> If failure occurs after called read_lock(), need call read_unlock() too.
> 
> It can fail in multiple position, so add new tag 'fail_lock' for it
> (also can let 'if' only content one jump statement).

You know, this is getting too frequent...  You really need to do
something about it.  OK, you've formed a hypothesis (in this case,
that ptrace_do_wait() returns non-zero with tasklist_lock still held).
If that hypothesis was correct, you would've found a bug and yes,
this patch would probably be more or less a fix for that bug.

Do you see what's missing?  That's right, verifying that hypothesis.
Which isn't hard to do, either by slapping a printk into these
exits, or by trying to build a proof.  As it is, hypothesis is
incorrect and your patch introduces breakage.  The same would have
happened if _some_ exits from that function returned non-zero
values with tasklist_lock held and some returned non-zero values
with tasklist_lock released.

You really need to realize that pattern-matching is not enough - you
need to prove that your fix is correct and that requires an analysis
of what's there.

"I see something odd" is a good reason to ask or to try and figure out
what's going on.  It's not a good reason for blindly making changes
like that - not until you've done the analysis and can at least show
that it won't _break_ things.

  reply	other threads:[~2013-10-05  6:35 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-05  5:53 [PATCH] kernel/exit.c: call read_unlock() when failure occurs after already called read_lock() in do_wait() Chen Gang
2013-10-05  6:34 ` Al Viro [this message]
2013-10-05  7:33   ` Chen Gang
2013-10-05 15:48     ` Al Viro
2013-10-05 16:48       ` Chen Gang

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=20131005063431.GU13318@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=akpm@linux-foundation.org \
    --cc=ebiederm@xmission.com \
    --cc=fweisbec@gmail.com \
    --cc=gang.chen@asianux.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=oleg@redhat.com \
    /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.