public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Paul Jackson <pj@sgi.com>
To: Jeremy Fitzhardinge <jeremy@goop.org>
Cc: srinivasa@in.ibm.com, linux-kernel@vger.kernel.org,
	akpm@linux-foundation.org, torvalds@linux-foundation.org,
	vatsa@in.ibm.com, dino@in.ibm.com, simon.derr@bull.net,
	clameter@sgi.com, clameter@cthulhu.engr.sgi.com,
	rientjes@google.com
Subject: Re: [RFC] [PATCH] cpuset operations causes Badness at mm/slab.c:777 warning
Date: Fri, 1 Jun 2007 13:44:07 -0700	[thread overview]
Message-ID: <20070601134407.6e4d72de.pj@sgi.com> (raw)
In-Reply-To: <466081DE.70205@goop.org>

> I think this is a good example of why having to special-case kmalloc(0)
> is a bad idea.  The original code was straightforward and, barring
> silliness, should be completely correct with npids==0.  This new code
> does nothing other than make things more complex.

Perhaps so.  Perhaps not.

Actually, the original cpuset pid_array_load() code was not correct,
and Christoph's work is smoking this out.

The original code assumed that it could access the first element of the
kmalloc'd array, before it checked if it could go even further.

But with the kmalloc call of:

	kmalloc(npids * sizeof(pid_t), GFP_KERNEL);

it is impossible for kmalloc to know how much to allocate, to guarantee
such behaviour.  If npids is zero, all kmalloc sees, in essence, is the
call:

	kmalloc(0, GFP_KERNEL);

There is no way it can guess that I might still want to access the
first "size(pid_t)" bytes of whatever it returned.

If we had a different sort of kmalloc API, such as would be called with:

	kmalloc(npids, sizeof(pid_t), GFP_KERNEL);

rather like the libc calloc() routine, having separate count and size
fields, then in theory, kmalloc could treat calls of the form:

	kmalloc(0, sz, GFP_KERNEL);	/* for sz > 0 */

as if they were actually:

	kmalloc(1, sz, GFP_KERNEL);

and then my cpuset code might not have been broken.  But, since
some of us can never remember whether it is the size or the count
that comes first in such an API, we would still have had such
bugs -- just fewer, for those who got those arguments backwards,
and finally tripped over this anyway.

That doesn't address the other likely broken code, elsewhere in
the kernel somewhere that happened to try to access the 2nd or
3rd element of such an empty array before noticing.

So ... while I started this reply agreeing with you, I end up still
agreeing with Christoph's changes here.

Thanks, Christoph, for finding a bug in the cpuset code - good work!

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

  reply	other threads:[~2007-06-01 20:44 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-06-01  7:27 [RFC] [PATCH] cpuset operations causes Badness at mm/slab.c:777 warning Srinivasa Ds
2007-06-01 10:50 ` Srinivasa Ds
2007-06-01 18:13   ` Christoph Lameter
2007-06-01 19:11     ` Paul Jackson
2007-06-01 19:18       ` Christoph Lameter
2007-06-01 19:47         ` Paul Jackson
2007-06-01 19:51           ` Christoph Lameter
2007-06-01 20:02             ` Paul Jackson
2007-06-01 20:06               ` Christoph Lameter
2007-06-01 20:19                 ` Paul Jackson
2007-06-01 20:43                   ` Christoph Lameter
2007-06-01 20:54                     ` Paul Jackson
2007-06-01 20:30   ` Jeremy Fitzhardinge
2007-06-01 20:44     ` Paul Jackson [this message]
2007-06-01 20:47     ` Christoph Lameter
2007-06-01 20:56       ` Jeremy Fitzhardinge
2007-06-01 20:59       ` Andrew Morton
2007-06-01 21:45         ` Christoph Lameter
2007-06-01 22:16           ` Andrew Morton
2007-06-01 22:20             ` Christoph Lameter
2007-06-01 22:33               ` Andrew Morton
2007-06-01 22:41                 ` Christoph Lameter
2007-06-01 23:00                   ` Linus Torvalds
2007-06-01 23:29                     ` Christoph Lameter
2007-06-01 23:41                       ` Linus Torvalds
2007-06-01 23:46                         ` Christoph Lameter
2007-06-01 23:57                           ` Linus Torvalds
2007-06-02  0:12                             ` Christoph Lameter
2007-06-02  0:16                             ` Andrew Morton
2007-06-02  0:26                               ` Christoph Lameter
2007-06-02  1:04                                 ` Linus Torvalds
2007-06-02  0:46                     ` Jeremy Fitzhardinge
2007-06-02  1:05                     ` Valdis.Kletnieks
2007-06-02  1:24                       ` Christoph Lameter
2007-06-01 23:02                   ` Andrew Morton
2007-06-01 23:16                     ` Christoph Lameter
2007-06-01 23:21                       ` Christoph Lameter
2007-06-01 23:36                       ` Linus Torvalds
2007-06-01 23:42                         ` Christoph Lameter
2007-06-01 23:25                     ` Linus Torvalds
2007-06-02  0:41                   ` Jeremy Fitzhardinge
2007-06-02  0:43             ` Jeremy Fitzhardinge
2007-06-02  0:51               ` Andrew Morton
2007-06-02  0:59                 ` Jeremy Fitzhardinge

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=20070601134407.6e4d72de.pj@sgi.com \
    --to=pj@sgi.com \
    --cc=akpm@linux-foundation.org \
    --cc=clameter@cthulhu.engr.sgi.com \
    --cc=clameter@sgi.com \
    --cc=dino@in.ibm.com \
    --cc=jeremy@goop.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rientjes@google.com \
    --cc=simon.derr@bull.net \
    --cc=srinivasa@in.ibm.com \
    --cc=torvalds@linux-foundation.org \
    --cc=vatsa@in.ibm.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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox