fix sys cpumap for > 352 NR_CPUS

Due to a bug in the 2.6 kernel, the cpumap files, such as:

/sys/devices/system/node/node1/cpumap

only display the high order 352 CPUs. If you compile a kernel with NR_CPUS=512, such as SGI's Altix is usually compiled, then the low order 512-352 == 160 CPU positions are not shown.

This bug breaks Andi Kleens numactl and libnuma user code which depends on this file presenting correctly. Andi has reviewed this fix, and it builds, boots and fixes the breakage (ia64 with sn2_defconfig).

Please apply the following patch (a hack, but better than the hack that was there before).

===================================================================

@@ -21,9 +21,21 @@ static ssize_t node_read_cpumap(struct s
 	cpumask_t mask = node_dev->cpumap;
 	int len;
 
-	/* FIXME - someone should pass us a buffer size (count) or
-	 * use seq_file or something to avoid buffer overrun risk. */
-	len = cpumask_scnprintf(buf, 99 /* XXX FIXME */, mask);
+	/*
+	 * Hack alert:
+	 * 1) This could overwrite a buffer w/o warning.  Someone should
+	 *     pass us a buffer size (count) or use seq_file or something
+	 *     to avoid buffer overrun risks.
+	 * 2) This can return a count larger than the read size requested
+	 *     by the user code - possibly confusing it.
+	 * 3) Following hardcodes that mask scnprintf format requires 9
+	 *     chars of output for each 32 bits of mask or fraction.
+	 * 4) Following prints stale node_dev->cpumap value, instead of
+	 *     evaluating afresh node_to_cpumask(node_dev->sysdev.id).
+	 * 5) Why does struct node even has the field cpumap.  Won't it
+	 *     just get stale, especially in the face of cpu hotplug?
+	 */
+	len = cpumask_scnprintf(buf, ((NR_CPUS+31)/32)*9 /* XXX FIXME */, mask);
 	len += sprintf(buf + len, "\n");
 	return len;
 }

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

We could - either way works about as well. Is there something special about PAGE_SIZE here? Is that in fact what sysfs is making available?

I can send a PAGE_SIZE hack-a-shaq if you like, or you can just code it yourself. If I do it, I will also fix the comment as appropriate. Whatever you prefer ...

	len = cpumask_scnprintf(buf, PAGE_SIZE /* XXX FIXME */, mask);

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

Think so. Greg, can you confirm that a SYSDEV_ATTR's handler can safely assume that it has a PAGE_SIZE buffer to write to?

Yes, that is correct.

thanks,

greg k-h

Good - thanks, Greg.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

That doesn't make sense.

My node_read_cpumap() routine is being passed a finite length buffer into which it is supposed to put some characters. Unless by contract or passed value it knows the length of that buffer, it cannot safely know how far it can write.

Apparently, from Andrews comments and from the line:

  buffer->page = (char *) get_zeroed_page(GFP_KERNEL);

in file.c:fill_read_buffer(), the length is PAGE_SIZE, by contract.

Greg - perhaps a comment in include/linux/sysdev.h, near the declarations for the show() and store() routines, specifying the buffer sizing, would be appropriate?

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

Then I apologize.

Please allow me to demonstrate with code, which should be clearer.

Name: Fix sysfs Node Cpumap for Large NR_CPUS
Status: Booted on 2.6.7-rc2-bk3

As pointed out by Paul Jackson, sometimes 99 chars is not enough. We currently get a page from sysfs: that code should check we don't haven't overrun it, and for futureproofing, detect problem at buildtime.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .9869-linux-2.6.7-rc2-bk3/drivers/base/node.c .9869-linux-2.6.7-rc2-bk3.updated/drivers/base/node.c
@@ -21,9 +21,10 @@ static ssize_t node_read_cpumap(struct s
 	cpumask_t mask = node_dev->cpumap;
 	int len;
 
-	/* FIXME - someone should pass us a buffer size (count) or
-	 * use seq_file or something to avoid buffer overrun risk. */
-	len = cpumask_scnprintf(buf, 99 /* XXX FIXME */, mask);
+	/* 2004/06/03: buf currently PAGE_SIZE, need > 1 char per 4 bits. */
+	BUILD_BUG_ON(NR_CPUS/4 > PAGE_SIZE/2);
+
+	len = cpumask_scnprintf(buf, -1UL, mask);
 	len += sprintf(buf + len, "\n");
 	return len;
 }

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .9869-linux-2.6.7-rc2-bk3/fs/sysfs/file.c .9869-linux-2.6.7-rc2-bk3.updated/fs/sysfs/file.c
@@ -89,6 +90,7 @@ static int fill_read_buffer(struct file 
 		return -ENOMEM;
 
 	count = ops->show(kobj,attr,buffer->page);
+	BUG_ON(count > PAGE_SIZE);
 	if (count >= 0)
 		buffer->count = count;
 	else

Anyone who quotes me in their signature is an idiot -- Rusty Russell

Whatever, I like BUILD_BUG_ON().

Honestly, I dislike the static check altogether, since it's tied too closely to the implementation of independent code. Since glibc will break on > 1024 cpus, I'm not too concerned about the 10000+ CPU limit.

Because now you have silently truncated, which is much worse than blowing up loudly. My version also works correctly when sysfs starts handing out two pages rather than one because something else needed it.

If you want to do this, then please do:

	len = cpumask_scnprintf(buf, PAGE_SIZE, mask);
	if (len == PAGE_SIZE)
		return -ENOMEM;

That would be extremely unusual; we tend not to panic even when things are bad. In this case, things are bad because of a static configuration error, so I find it hard to really care...

I'll do it; seems like we need negotiation with Greg-KH, too.

That's not true. I have faith in your abilities; I question anyone's ability to produce a perfectly balanced solution without any external input.

Rusty.

Anyone who quotes me in their signature is an idiot -- Rusty Russell

The build time check was your idea in the first place, as I recall. I hadn't added it to my variants. Apparently we agree not to add it. Ok.

I absolutely agree with your dislike of hidden intermittent failures.

For a constant failure such as this, even if everyone misses the botch for the first few times that SGI boots a bazillion CPU system in the lab, it will get noticed soon enough. This is in fact exactly what happened with the 99 char limit that was there now.

That looks nice.

If you want me to add the "if (len ..." check, let me know. Or if you want to send Andrew a patch that adds it, I'll gladly support that.

==> Do note that I had to change the -1UL to PAGE_SIZE, in a
patch to Andrew about 12 hours ago. The *scnprintf() family of fine formatting functions suppresses all requested output if handed a length with the high bit set.

Yup - I think it was Greg who said the same thing. Clearly this is not a panic.

I was wrong to suggest panic'ing here.

Ok - have fun.

Whatever ... my view of who was saying and doing what here doesn't entirely match yours.

So be it.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

This patch (fix-sys-cpumap-for-352-nr_cpus) doesn't work. Passing -1UL to cpumask_scnprintf() suppresses all of the requested output, because of the following code in lib/vsprintf.c vsnprintf():

        /* Reject out-of-range values early */
        if (unlikely((int) size < 0)) {
                /* There can be only one.. */
                static int warn = 1;
                WARN_ON(warn);
                warn = 0;
                return 0;
        }

Using PAGE_SIZE works ok. Well, lets throw in a -1 to leave a slot for the newline, while we're here.

Index: 2.6.7-rc2-mm2/drivers/base/node.c
===================================================================
@@ -24,7 +24,7 @@ static ssize_t node_read_cpumap(struct s
 	/* 2004/06/03: buf currently PAGE_SIZE, need > 1 char per 4 bits. */
 	BUILD_BUG_ON(NR_CPUS/4 > PAGE_SIZE/2);
 
-	len = cpumask_scnprintf(buf, -1UL, mask);
+	len = cpumask_scnprintf(buf, PAGE_SIZE-1, mask);
 	len += sprintf(buf + len, "\n");
 	return len;
 }

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

Ah yes ...

Apparently, Rusty, you are unfamiliar with the style of coding that involves "put in an ugly hack now, commenting the known open issues, and then clean up the mess some other day".

This is good ... good that you are unfamiliar with such.

Rather than try to convert you to my corrupt ways, which would be a great loss to the Linux community, and probably not possible anyway, I will:

1) Apologize - yes - I should have tried to ask these questions now and get this code right (which it turns out is happening anyway).

2) Send another patch to Andrew, with PAGE_SIZE as he recommends, and with the stale struct node cpumap field nuked, as you recommend.

Thank-you. It's a pleasure.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

Known open issues I have no problems with. It's asking questions in the comments that I disagree with: either you should ask on lkml, or grep.

To be honest, if you can't figure out the answers, you have to ask if you're the right person to be modifying the code.

Rusty.

Anyone who quotes me in their signature is an idiot -- Rusty Russell

We do check for an error in that function, so returning any negative error value for a show() sysfs callback will be handled properly.

thanks,

greg k-h

If a show() function returns an error, you handle it - good. As it should be.

But if a show() function overwrites the page buffer provided to it, before returning, then there is nothing you can do beyond sending condolences to the next of kin.

This PATCH and email thread came about because the buffer size is not passed into the show() function, nor, so far as I can tell, is it documented anywhere other than implicitly in the fill_read_buffer() code:

    buffer->page = (char *) get_zeroed_page(GFP_KERNEL);

So we were getting confused as to what size buffer we had, when coding defensively to avoid buffer overrun.

                          I won't rest till it's the best ...
                          Programmer, Linux Scalability
                          Paul Jackson <[email protected]> 1.650.933.1373

Yup, you broke the kernel, you get to keep both pieces :)

I agree that we should document this better. This thread has shown that.

Panicing the kernel is not really acceptable, even if you did just scribble accross any portion of kernel memory in your show() function. Just be aware of the size and code your show() function to be defensive and not overrun that size.

thanks,

greg k-h

This is where we have a philosophical difference. As I understand it, the rule is, "don't put big things in attributes". If we want to change that rule, we need to do more work, like pass the length to the show function, and handle -ENOMEM by reallocating and looping.

But I think the /rule/ is a good one: if you need to handle something arbitrarily large, DON'T USE THIS INTERFACE, because there is no way to do that correctly. This allows us to handle 99.9% of cases as a one-liner, which I think has great merit.

I think we should guarantee any kernel primitive fits into the space:
this means it should comfortably fit printing a cpumask_t. I would argue for a #error inside the cpumask or sysfs code which ensures we can fit two cpumasks (~7000 CPUs on page-size 4096), so we explode early if this ever becomes a problem, and a runtime sanity check inside the sysfs code to BUG on overrun.

If the average code is worrying about the exact size of the sysfs buffer, I think they're on the wrong path. If they are checking it at runtime, they've got a bug.

I hope this clarifies my thinking, and I can understand that people would disagree with my premise: that's OK.

Cheers,
Rusty.

Anyone who quotes me in their signature is an idiot -- Rusty Russell

We don't want to change the rule, it's a good rule.

Agreed.

Again, this seems to be a issue with the code that is trying to export a cpumask_t. I suggest you keep the check inside that code and keep it out of the sysfs core, which does not need it for 99.99% of the cases.

thanks,

greg k-h