From: Chen Gang <gang.chen@asianux.com>
To: Al Viro <viro@ZenIV.linux.org.uk>
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, 05 Oct 2013 15:33:40 +0800 [thread overview]
Message-ID: <524FC0D4.9070407@asianux.com> (raw)
In-Reply-To: <20131005063431.GU13318@ZenIV.linux.org.uk>
On 10/05/2013 02:34 PM, Al Viro wrote:
> 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.
>
>
Oh, it is my fault, this is incorrect patch. Hmm... I realize a mistake
of me: I have said "when finding issues, I need consider about LTP in q4
2013, need let it can be tested by LTP".
And you feel "this is getting too frequent...", can you provide my
failure/succeed ratio?
Or for a short proof: next, I will try to find 2 patches by reading code
within "./kernel" sub-directory, if all of them are incorrect, I will
*never* send patches again by reading code. Is it OK?
Hmm... but all together, I still will use compiler and test tools to
find/solve issues (I have found 3-4 issues by LTP test tools, now just
analyzing them, although I am not sure they must be kernel's issue).
Thanks.
--
Chen Gang
next prev parent reply other threads:[~2013-10-05 7:34 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
2013-10-05 7:33 ` Chen Gang [this message]
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=524FC0D4.9070407@asianux.com \
--to=gang.chen@asianux.com \
--cc=akpm@linux-foundation.org \
--cc=ebiederm@xmission.com \
--cc=fweisbec@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=oleg@redhat.com \
--cc=viro@ZenIV.linux.org.uk \
/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.