-stable review

This is the start of the stable review cycle for the 2.6.11.7 release. There are 8 patches in this series, all will be posted as a response to this one. If anyone has any issues with these being applied, please let us know. If anyone is a maintainer of the proper subsystem, and wants to add a Signed-off-by: line to the patch, please respond with it.

These patches are sent out with a number of different people on the Bcc:
line. If you wish to be a reviewer, please email [email protected] to add your name to the list. If you want to be off the reviewer list, also email us.

Responses should be made by Thursday, April 7, 17:00 UTC. Anything received after that time, might be too late.

thanks,

the -stable release team

-stable review patch. If anyone has any objections, please let us know.

------------------

the patch below fixes the bug of ALSA timer notification, which is used in the recent ALSA dmix plugin.

- fixed Oops in read()
- fixed wake-up polls and signals with new events

@@ -1117,7 +1117,8 @@
 	if (tu->qused >= tu->queue_size) {
 		tu->overrun++;
 	} else {
-		memcpy(&tu->queue[tu->qtail++], tread, sizeof(*tread));
+		memcpy(&tu->tqueue[tu->qtail++], tread, sizeof(*tread));
+		tu->qtail %= tu->queue_size;
 		tu->qused++;
 	}
 }

@@ -1140,6 +1141,8 @@
 	spin_lock(&tu->qlock);
 	snd_timer_user_append_to_tqueue(tu, &r1);
 	spin_unlock(&tu->qlock);
+	kill_fasync(&tu->fasync, SIGIO, POLL_IN);
+	wake_up(&tu->qchange_sleep);
 }
 
 static void snd_timer_user_tinterrupt(snd_timer_instance_t *timeri,

-------------------------------------------------------
SF email is sponsored by - The IT Product Guide
Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now.
http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click

-stable review patch. If anyone has any objections, please let us know.

------------------

This patch from Stephen Tweedie which fixes a race in jbd code (it demonstrated itself as more or less random NULL dereferences in the journal code).

@@ -1775,10 +1775,10 @@ static int journal_unmap_buffer(journal_
 			JBUFFER_TRACE(jh, "checkpointed: add to BJ_Forget");
 			ret = __dispose_buffer(jh,
 					journal->j_running_transaction);
+			journal_put_journal_head(jh);
 			spin_unlock(&journal->j_list_lock);
 			jbd_unlock_bh_state(bh);
 			spin_unlock(&journal->j_state_lock);
-			journal_put_journal_head(jh);
 			return ret;
 		} else {
 			/* There is no currently-running transaction. So the

@@ -1789,10 +1789,10 @@ static int journal_unmap_buffer(journal_
 				JBUFFER_TRACE(jh, "give to committing trans");
 				ret = __dispose_buffer(jh,
 					journal->j_committing_transaction);
+				journal_put_journal_head(jh);
 				spin_unlock(&journal->j_list_lock);
 				jbd_unlock_bh_state(bh);
 				spin_unlock(&journal->j_state_lock);
-				journal_put_journal_head(jh);
 				return ret;
 			} else {
 				/* The orphan record's transaction has

@@ -1813,10 +1813,10 @@ static int journal_unmap_buffer(journal_
 					journal->j_running_transaction);
 			jh->b_next_transaction = NULL;
 		}
+		journal_put_journal_head(jh);
 		spin_unlock(&journal->j_list_lock);
 		jbd_unlock_bh_state(bh);
 		spin_unlock(&journal->j_state_lock);
-		journal_put_journal_head(jh);
 		return 0;
 	} else {
 		/* Good, the buffer belongs to the running transaction.

-stable review patch. If anyone has any objections, please let us know.

------------------

Attached is a patch against David's audit.17 kernel that adds checks for the TIF_SYSCALL_AUDIT thread flag to the ia64 system call and signal handling code paths. The patch enables auditing of system calls set up via fsys_bubble_down, as well as ensuring that audit_syscall_exit() is called on return from sigreturn.

Neglecting to check for TIF_SYSCALL_AUDIT at these points results in incorrect information in audit_context, causing frequent system panics when system call auditing is enabled on an ia64 system.

I have tested this patch and have seen no problems with it.

[Original patch from Amy Griffis ported to current kernel by David Woodhouse]

From: Amy Griffis <[email protected]>
From: David Woodhouse <[email protected]>

@@ -611,8 +611,10 @@
 	movl r2=ia64_ret_from_syscall
 	;;
 	mov rp=r2				// set the real return addr
-	tbit.z p8,p0=r3,TIF_SYSCALL_TRACE
+	and r3=_TIF_SYSCALL_TRACEAUDIT,r3
 	;;
+	cmp.eq p8,p0=r3,r0
+
 (p10)	br.cond.spnt.many ia64_ret_from_syscall	// p10=true means out registers are more than 8
 (p8)	br.call.sptk.many b6¶		// ignore this return addr
 	br.cond.sptk ia64_trace_syscall

=== arch/ia64/kernel/signal.c 1.49 vs edited ==--- 1.49/arch/ia64/kernel/signal.c 2005-01-25 20:23:45 +00:00
+++ edited/arch/ia64/kernel/signal.c 2005-04-01 00:18:29 +01:00
@@ -224,7 +224,8 @@
* could be corrupted.
*/
retval = (long) &ia64_leave_kernel;
- if (test_thread_flag(TIF_SYSCALL_TRACE))
+ if (test_thread_flag(TIF_SYSCALL_TRACE)
+ || test_thread_flag(TIF_SYSCALL_AUDIT))
/*
* strace expects to be notified after sigreturn returns even though the
* context to which we return may not be in the middle of a syscall.

Greg> -stable review patch. If anyone has any objections, please Greg> let us know.

Nitpick: the patch introduces trailing whitespace.

Why doesn't everybody use emacs and enable show-trailing-whitespace? ;-)

	--david

Sorry about that, I've removed it from the patch now.

Because some of us use vim and ":set list" to see it, when we remember to... :)

thanks,

greg k-h

others check received patches with a script instead of.... no, let's not debate $EDITOR.

~Randy

Try adding this to your .vimrc:

highlight WhitespaceEOL ctermbg=red guibg=red
match WhitespaceEOL /\s\+$/

Then you'll have to resist the urge to fix whitespace issues instead of not seeing them at all.

Ryan Anderson
  sometimes Pug Majere

Very nice, thanks a lot for that.

greg k-h

let c_space_errors=1 also works great.

Dave

Yeah, that's a risk. But gratuitous trailing whitespace changes shouldn't cause a lot of downstream problems due to `patch -l'.

What I do is to ensure that we never _add_ trailing whitespace. So anything which matches

	^+.*[tab or space]$

gets trimmed. My theory is that after 10 years of this, all the trailing whitespace will be gone. Problem is, I also see the hundreds of lines of code in the bk patches which add trailing whitespace :(

Larry sent me a little bk script which would spam the user if they tried to commit something which adds trailing whitespace, but maybe that's a bit academic right now.

-stable review patch. If anyone has any objections, please let us know.

------------------

This fixes an oops in the eeprom driver. It was first reported here:
http://bugzilla.kernel.org/show_bug.cgi?id=4347

It was additionally discussed here (while tracking a completely different bug):
http://archives.andrew.net.au/lm-sensors/msg30021.html

The patch is already in 2.6.12-rc1:
http://linux.bkbits.net:8080/linux-2.5/[email protected]

The oops happens when one reads data from the sysfs interface file such that (off < 16) and (count < 16 - off). For example "sensors" from lm_sensors 2.9.0 does this, and causes the oops.

@@ -130,7 +130,8 @@
 
 	/* Hide Vaio security settings to regular users (16 first bytes) */
 	if (data->nature == VAIO && off < 16 && !capable(CAP_SYS_ADMIN)) {
-		int in_row1 = 16 - off;
+		size_t in_row1 = 16 - off;
+		in_row1 = min(in_row1, count);
 		memset(buf, 0, in_row1);
 		if (count - in_row1 > 0)
 			memcpy(buf + in_row1, &data->data[16], count - in_row1);

-stable review patch. If anyone has any objections, please let us know.

------------------

This patch from Herbert Xu fixes a deadlock with IPsec. When an ICMP frag. required is sent and the ICMP message needs the same SA as the packet that caused it the state will be locked twice.

[IPSEC]: Do not hold state lock while checking size.

This can elicit ICMP message output and thus result in a deadlock.

diff -Nru a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
@@ -103,16 +103,16 @@
 			goto error_nolock;
 	}
 
-	spin_lock_bh(&x->lock);
-	err = xfrm_state_check(x, skb);
-	if (err)
-		goto error;
-
 	if (x->props.mode) {
 		err = xfrm4_tunnel_check_size(skb);
 		if (err)
-			goto error;
+			goto error_nolock;
 	}
+
+	spin_lock_bh(&x->lock);
+	err = xfrm_state_check(x, skb);
+	if (err)
+		goto error;
 
 	xfrm4_encap(skb);
 

diff -Nru a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
@@ -103,16 +103,16 @@
 			goto error_nolock;
 	}
 
-	spin_lock_bh(&x->lock);
-	err = xfrm_state_check(x, skb);
-	if (err)
-		goto error;
-
 	if (x->props.mode) {
 		err = xfrm6_tunnel_check_size(skb);
 		if (err)
-			goto error;
+			goto error_nolock;
 	}
+
+	spin_lock_bh(&x->lock);
+	err = xfrm_state_check(x, skb);
+	if (err)
+		goto error;
 
 	xfrm6_encap(skb);
 

-stable review patch. If anyone has any objections, please let us know.

------------------

We should merge this backport - it's needed to prevent deadlocks when dio_complete() does up_read() from IRQ context. And perhaps other places.

From: David Howells <[email protected]>

[PATCH] rwsem: Make rwsems use interrupt disabling spinlocks

The attached patch makes read/write semaphores use interrupt disabling spinlocks in the slow path, thus rendering the up functions and trylock functions available for use in interrupt context. This matches the regular semaphore behaviour.

I've assumed that the normal down functions must be called with interrupts enabled (since they might schedule), and used the irq-disabling spinlock variants that don't save the flags.

diff -Nru a/lib/rwsem-spinlock.c b/lib/rwsem-spinlock.c
@@ -140,12 +140,12 @@
 
 	rwsemtrace(sem, "Entering __down_read");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irq(&sem->wait_lock);
 
 	if (sem->activity >= 0 && list_empty(&sem->wait_list)) {
 		/* granted */
 		sem->activity++;
-		spin_unlock(&sem->wait_lock);
+		spin_unlock_irq(&sem->wait_lock);
 		goto out;
 	}
 

@@ -160,7 +160,7 @@
 	list_add_tail(&waiter.list, &sem->wait_list);
 
 	/* we don't need to touch the semaphore struct anymore */
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irq(&sem->wait_lock);
 
 	/* wait to be given the lock */
 	for (;;) {

@@ -181,10 +181,12 @@
  */
 int fastcall __down_read_trylock(struct rw_semaphore *sem)
 {
+	unsigned long flags;
 	int ret = 0;
+
 	rwsemtrace(sem, "Entering __down_read_trylock");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	if (sem->activity >= 0 && list_empty(&sem->wait_list)) {
 		/* granted */

@@ -192,7 +194,7 @@
 		ret = 1;
 	}
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving __down_read_trylock");
 	return ret;

@@ -209,12 +211,12 @@
 
 	rwsemtrace(sem, "Entering __down_write");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irq(&sem->wait_lock);
 
 	if (sem->activity == 0 && list_empty(&sem->wait_list)) {
 		/* granted */
 		sem->activity = -1;
-		spin_unlock(&sem->wait_lock);
+		spin_unlock_irq(&sem->wait_lock);
 		goto out;
 	}
 

@@ -229,7 +231,7 @@
 	list_add_tail(&waiter.list, &sem->wait_list);
 
 	/* we don't need to touch the semaphore struct anymore */
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irq(&sem->wait_lock);
 
 	/* wait to be given the lock */
 	for (;;) {

@@ -250,10 +252,12 @@
  */
 int fastcall __down_write_trylock(struct rw_semaphore *sem)
 {
+	unsigned long flags;
 	int ret = 0;
+
 	rwsemtrace(sem, "Entering __down_write_trylock");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	if (sem->activity == 0 && list_empty(&sem->wait_list)) {
 		/* granted */

@@ -261,7 +265,7 @@
 		ret = 1;
 	}
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving __down_write_trylock");
 	return ret;

@@ -272,14 +276,16 @@
  */
 void fastcall __up_read(struct rw_semaphore *sem)
 {
+	unsigned long flags;
+
 	rwsemtrace(sem, "Entering __up_read");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	if (--sem->activity == 0 && !list_empty(&sem->wait_list))
 		sem = __rwsem_wake_one_writer(sem);
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving __up_read");
 }

@@ -289,15 +295,17 @@
  */
 void fastcall __up_write(struct rw_semaphore *sem)
 {
+	unsigned long flags;
+
 	rwsemtrace(sem, "Entering __up_write");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	sem->activity = 0;
 	if (!list_empty(&sem->wait_list))
 		sem = __rwsem_do_wake(sem, 1);
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving __up_write");
 }

@@ -308,15 +316,17 @@
  */
 void fastcall __downgrade_write(struct rw_semaphore *sem)
 {
+	unsigned long flags;
+
 	rwsemtrace(sem, "Entering __downgrade_write");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	sem->activity = 1;
 	if (!list_empty(&sem->wait_list))
 		sem = __rwsem_do_wake(sem, 0);
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving __downgrade_write");
 }

diff -Nru a/lib/rwsem.c b/lib/rwsem.c
@@ -150,7 +150,7 @@
 	set_task_state(tsk, TASK_UNINTERRUPTIBLE);
 
 	/* set up my own style of waitqueue */
-	spin_lock(&sem->wait_lock);
+	spin_lock_irq(&sem->wait_lock);
 	waiter->task = tsk;
 	get_task_struct(tsk);
 

@@ -163,7 +163,7 @@
 	if (!(count & RWSEM_ACTIVE_MASK))
 		sem = __rwsem_do_wake(sem, 0);
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irq(&sem->wait_lock);
 
 	/* wait to be given the lock */
 	for (;;) {

@@ -219,15 +219,17 @@
  */
 struct rw_semaphore fastcall *rwsem_wake(struct rw_semaphore *sem)
 {
+	unsigned long flags;
+
 	rwsemtrace(sem, "Entering rwsem_wake");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	/* do nothing if list empty */
 	if (!list_empty(&sem->wait_list))
 		sem = __rwsem_do_wake(sem, 0);
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving rwsem_wake");
 

@@ -241,15 +243,17 @@
  */
 struct rw_semaphore fastcall *rwsem_downgrade_wake(struct rw_semaphore *sem)
 {
+	unsigned long flags;
+
 	rwsemtrace(sem, "Entering rwsem_downgrade_wake");
 
-	spin_lock(&sem->wait_lock);
+	spin_lock_irqsave(&sem->wait_lock, flags);
 
 	/* do nothing if list empty */
 	if (!list_empty(&sem->wait_list))
 		sem = __rwsem_do_wake(sem, 1);
 
-	spin_unlock(&sem->wait_lock);
+	spin_unlock_irqrestore(&sem->wait_lock, flags);
 
 	rwsemtrace(sem, "Leaving rwsem_downgrade_wake");
 	return sem;

-stable review patch. If anyone has any objections, please let us know.

------------------

Since BIC is the default congestion control algorithm enabled in every 2.6.x kernel out there, fixing errors in it becomes quite critical.

A flaw in the loss handling caused it to not perform the binary search regimen of the BIC algorithm properly.

The fix below from Stephen Hemminger has been heavily verified.

[TCP]: BIC not binary searching correctly

While redoing BIC for the split up version, I discovered that the existing 2.6.11 code doesn't really do binary search. It ends up being just a slightly modified version of Reno. See attached graphs to see the effect over simulated 1mbit environment.

The problem is that BIC is supposed to reset the cwnd to the last loss value rather than ssthresh when loss is detected. The correct code (from the BIC TCP code for Web100) is in this patch.

@@ -1653,7 +1653,10 @@
 static void tcp_undo_cwr(struct tcp_sock *tp, int undo)
 {
 	if (tp->prior_ssthresh) {
-		tp->snd_cwnd = max(tp->snd_cwnd, tp->snd_ssthresh<<1);
+		if (tcp_is_bic(tp))
+			tp->snd_cwnd = max(tp->snd_cwnd, tp->bictcp.last_max_cwnd);
+		else
+			tp->snd_cwnd = max(tp->snd_cwnd, tp->snd_ssthresh<<1);
 
 		if (undo && tp->prior_ssthresh > tp->snd_ssthresh) {
 			tp->snd_ssthresh = tp->prior_ssthresh;

I hate to be a stickler for the rules, but does this really meet this criteria?

- It must fix a real bug that bothers people (not a, "This could be a problem..." type thing.)

If the congestion control alogirthm is "Reno-like", what is user-visible impact to users? There are OS's out there with TCP/IP stacks that are still using Reno, aren't there?

Knowing the answer to the question, "How does this bug `bother' either users or network administrators?" would probably be really helpful.

- Ted

An incorrect implementation of any congestion control algorithm has ramifications not considered when the congestion control author verified the design of his algorithm.

This has a large impact on every user on the internet, not just Linux machines.

Perhaps on a microscopic scale "this" part of the BIC algorithm was just behaving Reno-like due to the bug, but what implications does that error have as applied to the other heuristics in BIC? This is what I'm talking about. BIC operates in several modes, one of which is a pseudo binary search mode, and another is a less aggressive slower increase mode.

Therefore I think fixes to congestion control algorithms which are enabled by default always should take a high priority in the stable kernels.

Also, hopefully distro vendors will pick up 2.6.11.X fixes and update their customers.

-stable review patch. If anyone has any objections, please let us know.

------------------

Uses __va_copy instead of va_copy since some old versions of gcc (2.95.4 for instance) don't accept va_copy.

---

 clean-linux-2.6.11-paolo/arch/um/kernel/skas/uaccess.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletion(-)

diff -puN arch/um/kernel/skas/uaccess.c~uml-va_copy_fix arch/um/kernel/skas/uaccess.c
@@ -61,7 +61,8 @@ static void do_buffer_op(void *jmpbuf, v
 	void *arg;
 	int *res;
 
-	va_copy(args, *(va_list *)arg_ptr);
+	/* Some old gccs recognize __va_copy, but not va_copy */
+	__va_copy(args, *(va_list *)arg_ptr);
 	addr = va_arg(args, unsigned long);
 	len = va_arg(args, int);
 	is_write = va_arg(args, int);

_

_______________________________________________
stable mailing list
[email protected]
http://linux.kernel.org/mailman/listinfo/stable

Are there many kernels still being built with 2.95.4? It's quite antiquated, as far as i'm aware.

The use of '__' violates compiler namespace. If 2.95.4 were not easily replaced by a much better version (3.3.x? 3.4.x) I would see a reason to disregard this, but a fix merely to satisfy an obsolete compiler?

In my humblest of opinions you are fixing a bug that is better solved by downloading a more recent version of gcc.

Regards,

Renate Meijer.

Why? The symbol is defined by the compiler itself.

Let's not flame, Linus Torvalds said "we support GCC 2.95.3, because the newer versions are worse compilers in most cases". One user complained, even because he uses Debian, and I cannot do less than make sure that we comply with the requirements we have choosen (compiling with that GCC).

Please let's not start a flame on this. Consider me as having no opinion on this except not wanting to break on purpose Debian users. If you want, submit a patch removing Gcc 2.95.3 from supported versions, and get ready to fight for it (and probably loose).

Also, that GCC has discovered some syscall table errors in UML - I sent a separate patch, which was a bit big sadly (in the reduced version, about 70 lines + description).

Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

If a function is prefixed with a double underscore, this implies the function is internal to the compiler, and may change at any time, since it's not governed by some sort of standard. Hence that code may start suffering from bitrot and complaining to the compiler guys won't help.

They'll just tell you to RTFM.

You make it sound as if you were reciting Ye Holy Scribings. When did Linus Thorvalds say this? In the Redhat-2.96 debacle? Before or after 3.3? I have searched for that quote, but could not find it, and having suffered under 3.1.1, I can well understand his wearyness for the earlier versions.

See

http://kerneltrap.org/node/4126, halfway down.

For the cold, hard facts...

http://www.suse.de/~aj/SPEC/

<snip>

If Debian users are stuck with a pretty outdated compiler, i'd seriously suggest migrating to some
other distro which allows more freedom. If linux itself is holding them back, there's a need for some serious patching. If there are serious issues in the gcc compiler, which hinder migration to a more up-to-date version our efforts should be directed at solving them in that project, not this.

I don't fight over things like that, i'm not interested in politics. I merely point out the problem. And yes.
I do think support for obsolete compiler should be dumped in favor of a more modern version. Especially if that compiler requires invasions of compiler-namespace. The patch, as presented, is not guaranteed to be portable over versions, and may thus introduce another problem with future versions of GCC.

I am not quite sure what is intended here... Please explain.

timeo hominem unius libri

Thomas van Aquino

You did read include/linux/compiler.h, didn't you? And you did read this thread as well, right?
http://kerneltrap.org/node/4126

Jörn

Time? What's that? Time is only worth what you do with it.
-- Theo de Raadt

Yes. It seems a good place to hide compiler-internal stuff as the one this patch implements. And I know linux does not exactly have a great track record on this issue. But just because the dependency is there, does not imply it is a GOOD THING to have it.

So instead of applying this patch, simply

#ifdef VERSION_MINOR < WHATEVER
#define va_copy __va_copy
#endif

in include/linux/compiler_gcc2.h

Thus solving the problem without having to invade compiler namespace all over the place, but doing so in *one* place only.

<quote>
Things seem to have improved a bit lately. The gcc-3.x series was basically not worth it for plain C until 3.3 or so. </quote>

Yes. You did read the actual data as produced by that guy from Suse, did you? In the past, people may have justly stuck to (e.g.) 2.95.3, however, support for that version now starts to require dependencies on compiler internals. This is one argument in favor of dropping support for that version, or at least not to spread compiler dependent stuff all over the code.

Fyi, another fact that was missing from the quoted thread: gcc 2.95 catches bugs that 3.x compilers simply miss. Support for the old compiler is more work, no doubt, and at times requires to work around plain compiler bugs as well. But there is some return on investment.

Is it worth the effort? Not sure. But the "it's old, drop support for it" argument just doesn't cut it and it doesn't get any better by repetition.

Jörn

Schrödinger's cat is <BLINK>not</BLINK> dead.
-- Illiad

Exactly, that's why this patch is valid.

thanks,

greg k-h

However, the argument gets better every time "a workaround" is needed. If there are still serious issues open (like a failure to catch bugs the old version did), they are issues which need resolving in the compiler. Patching the wrong project, is introducing two imperfections.

I think its worth the time and trouble to take this up with the gcc crowd. So if you could provide a list of things 3.3 misses, i'm sure the gcc-crowd would like it.

At the very least, it's at the wrong place, since it should be patched in ./include/linux/compiler.h. And I do not exactly argue "it's old, drop support for it", but rely on the "dont rely on compiler internals or at least stick them on one place where everyone can find them easily, instead of peppering the entire codebase with them" argument.

Regards,

Renate Meijer.

If you volunteer to do work with the gcc-crowd, I can dig up some old stuff and send you testcases. Sure.

Jörn

The strong give up and move away, while the weak give up and stay.
-- unknown

I'll volunteer. As I said, i think it's worth the time and trouble. But i can't do it on my own, at least i need some backup from thee development community around here. You telling me what's up, for one. I found one mail by Greg KH spelling out some of the problems, but you suggest there's more to worry about.

Problem is, i'll be spending 5 weeks prety much scattered around europe, starting next friday and have no idea on my online-ness yet. As it is, my trusty mac is my only digital companion, and my linux-box is in storage for the time being.

So don't expect any results before May 17. But hey... Somebody has to do it. Just don't be surprised if the folks at gcc do not agree with you "a prima vista". They may have a different idea on what exactly constitutes a bug.

Upside is, i'll be taking my Mac anyway, so at least i'll have the sources handy. I'll start downloading tonight, so if you have data, please, lets have it.

Nevertheless, the points made in previous posts stand.

Regards,

Renate Meijer.
--

timeo hominem unius libri

Thomas van Aquino

Thanks!

Have fun!

Ok, here is the first testcase. It was a real bug, even though never submitted to Linus:

#define ASM_MACRO	\
	op;		\
#ifdef FOO		\
	op;		\
#else			\
	op;		\
#endif			\
	op;		\
	op;

The thing occurred in some entry.S or head.S, don't remember exactly which one. Gcc people might tell you unfriendly things about using the _C_ preprocessor for _ASM_ code, but that's just how the kernel code is written.

With gcc 2.95, the old preprocessor errored out on the correct line and we had a look at the code. With 3.x, preprocessor chewed things and starting from "#ifdef", everything was interpreted as a comment and ignored.

Code never worked and the real bug was papered over by more ugliness, but stayed that way for a year until someone (me) tried to compile it with 2.95.

Jörn

You cannot suppose that Moliere ever troubled himself to be original in the
matter of ideas. You cannot suppose that the stories he tells in his plays
have never been told before. They were culled, as you very well know.
-- Andre-Louis Moreau in Scarabouche

About this one: thanks for suggesting this and being constructive, I'll do ASAP (if I don't forget) this for the -bk tree. However, I think that Greg KH for the stable tree would prefer a local tested patch rather than a global one with possible side effects, right Greg?

Also, I hope this discussion does not count as a vote for the -stable tree inclusion (since dropping GCC 2 support in the -stable tree is exactly the purpose of this tree, right ;-) ? ).

Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

For Jörn Engel and the issue he opened: at the end of this mail I describe another bug caught by 2.95 and not by 3.x.

Ok, agreed in general. However, the -stable tree is for "current" GCC. Your objections would better refer to the fact that the same patch has already been merged into the main trunk.

Also, they have no point in doing this, probably. And the __va_copy name was used in the draft C99 standard so it's widespread (I've read this on "man 3 va_copy").

Sorry for the quote marks, it was a resume of what he said (and from re-reading, it's still a correct resume).

I've read the same kerneltrap article you quote.

Ok, read.

Linus pointed out that SPEC performances are not a good test case for the kernel compilation in that article. Point out a kernel compilation case.

I guess they can, if they want, upgrade some selected packages from newer trees, maybe by recompiling (at least, on Gentoo it's trivial, maybe on a binary distro like Debian it's harder).

Linus spoke about the compiler speed, which isn't such a bad reason. He's unfair in saying that GCC 3.x does not optimize better than older releases, probably; I guess that the compilation flags (I refer to -fno-strict-aliasing, which disables some optimizations) make some difference, as do the memory barriers (as pointed in the comments).

When and if that will happen, I'll come with an hack. UML already has need for some GCC - version specific behaviour (see arch/um/kernel/gmon_syms.c on a recent BitKeeper snapshot, even -rc1-bk5 has this code).

I'm reattaching the patch, so that you can look at the changelog (I'm also resending it as a separate email so that it is reviewed and possibly merged). Basically this is an error in GCC 2 and not in GCC 3:

int [] list = {
[0] = 1,
[0] = 1
}
(I've not tested the above itself, but this should be a stripped down version of one of the bugs fixed in the patch).

That sort of code in the UML syscall table is not the safer one - in fact, apart this patch for the stable tree, I'm refactoring the UML syscall table completely (for 2.6.12 / 2.6.13).

Btw: I've not investigated which one of the two behaviours is the buggy one - if you know, maybe you or I can report it.

Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

*) Uml 2.6.11 does not compile with gcc 2.95.4 because some entries are duplicated, and that GCC does not accept this (unlike gcc 3). Plus various other bugs in the syscall table definitions:

*) 223 is a syscall hole (i.e. ni_syscall) only on i386, on x86_64 it's a valid syscall (thus a duplicated one).

*) __NR_vserver must be only once with sys_ni_syscall, and not multiple times with different values!

*) syscalls duplicated in SUBARCHs and in common files (thus assigning twice to the same array entry and causing the GCC 2.95.4 failure mentioned above):
sys_utimes, which is common, and sys_fadvise64_64, sys_statfs64, sys_fstatfs64, which exist only on i386.

*) syscalls duplicated in each SUBARCH, to put in common files:
sys_remap_file_pages, sys_utimes, sys_fadvise64

*) 285 is a syscall hole (i.e. ni_syscall) only on i386, on x86_64 the range does not arrive to that point.

*) on x86_64, the macro name is __NR_kexec_load and not __NR_sys_kexec_load. Use the correct name in either case.

Note: as you can see, part of the syscall table definition in UML is arch-independent (with everywhere defined syscalls), and part is arch-dependant. This has created confusion (some syscalls are listed in both places, some in the wrong one, some are wrong on one arch or another).

---

 clean-linux-2.6.11-paolo/arch/um/include/sysdep-i386/syscalls.h   |   12 +++++-----
 clean-linux-2.6.11-paolo/arch/um/include/sysdep-x86_64/syscalls.h |    5 ----
 clean-linux-2.6.11-paolo/arch/um/kernel/sys_call_table.c          |   11 +++------
 3 files changed, 10 insertions(+), 18 deletions(-)

diff -puN arch/um/include/sysdep-i386/syscalls.h~uml-quick-fix-syscall-table-for-stable arch/um/include/sysdep-i386/syscalls.h
@@ -23,6 +23,9 @@ extern long sys_mmap2(unsigned long addr
 		      unsigned long prot, unsigned long flags,
 		      unsigned long fd, unsigned long pgoff);
 
+/* On i386 they choose a meaningless naming.*/
+#define __NR_kexec_load __NR_sys_kexec_load
+
 #define ARCH_SYSCALLS \
 	[ __NR_waitpid ] = (syscall_handler_t *) sys_waitpid, \
 	[ __NR_break ] = (syscall_handler_t *) sys_ni_syscall, \

@@ -101,15 +104,12 @@ extern long sys_mmap2(unsigned long addr
 	[ 223 ] = (syscall_handler_t *) sys_ni_syscall, \
 	[ __NR_set_thread_area ] = (syscall_handler_t *) sys_ni_syscall, \
 	[ __NR_get_thread_area ] = (syscall_handler_t *) sys_ni_syscall, \
-	[ __NR_fadvise64 ] = (syscall_handler_t *) sys_fadvise64, \
 	[ 251 ] = (syscall_handler_t *) sys_ni_syscall, \
-        [ __NR_remap_file_pages ] = (syscall_handler_t *) sys_remap_file_pages, \
-	[ __NR_utimes ] = (syscall_handler_t *) sys_utimes, \
-	[ __NR_vserver ] = (syscall_handler_t *) sys_ni_syscall,
-        
+	[ 285 ] = (syscall_handler_t *) sys_ni_syscall,
+
 /* 222 doesn't yet have a name in include/asm-i386/unistd.h */
 
-#define LAST_ARCH_SYSCALL __NR_vserver
+#define LAST_ARCH_SYSCALL 285
 
 /*
  * Overrides for Emacs so that we follow Linus's tabbing style.

diff -puN arch/um/include/sysdep-x86_64/syscalls.h~uml-quick-fix-syscall-table-for-stable arch/um/include/sysdep-x86_64/syscalls.h
@@ -71,12 +71,7 @@ extern syscall_handler_t sys_arch_prctl;
 	[ __NR_iopl ] = (syscall_handler_t *) sys_ni_syscall, \
 	[ __NR_set_thread_area ] = (syscall_handler_t *) sys_ni_syscall, \
 	[ __NR_get_thread_area ] = (syscall_handler_t *) sys_ni_syscall, \
-        [ __NR_remap_file_pages ] = (syscall_handler_t *) sys_remap_file_pages, \
 	[ __NR_semtimedop ] = (syscall_handler_t *) sys_semtimedop, \
-	[ __NR_fadvise64 ] = (syscall_handler_t *) sys_fadvise64, \
-	[ 223 ] = (syscall_handler_t *) sys_ni_syscall, \
-	[ __NR_utimes ] = (syscall_handler_t *) sys_utimes, \
-	[ __NR_vserver ] = (syscall_handler_t *) sys_ni_syscall, \
 	[ 251 ] = (syscall_handler_t *) sys_ni_syscall,
 
 #define LAST_ARCH_SYSCALL 251

diff -puN arch/um/kernel/sys_call_table.c~uml-quick-fix-syscall-table-for-stable arch/um/kernel/sys_call_table.c
@@ -48,7 +48,6 @@ extern syscall_handler_t sys_vfork;
 extern syscall_handler_t old_select;
 extern syscall_handler_t sys_modify_ldt;
 extern syscall_handler_t sys_rt_sigsuspend;
-extern syscall_handler_t sys_vserver;
 extern syscall_handler_t sys_mbind;
 extern syscall_handler_t sys_get_mempolicy;
 extern syscall_handler_t sys_set_mempolicy;

@@ -242,6 +241,7 @@ syscall_handler_t *sys_call_table[] = {
 	[ __NR_epoll_create ] = (syscall_handler_t *) sys_epoll_create,
 	[ __NR_epoll_ctl ] = (syscall_handler_t *) sys_epoll_ctl,
 	[ __NR_epoll_wait ] = (syscall_handler_t *) sys_epoll_wait,
+	[ __NR_remap_file_pages ] = (syscall_handler_t *) sys_remap_file_pages,
         [ __NR_set_tid_address ] = (syscall_handler_t *) sys_set_tid_address,
 	[ __NR_timer_create ] = (syscall_handler_t *) sys_timer_create,
 	[ __NR_timer_settime ] = (syscall_handler_t *) sys_timer_settime,

@@ -252,12 +252,10 @@ syscall_handler_t *sys_call_table[] = {
 	[ __NR_clock_gettime ] = (syscall_handler_t *) sys_clock_gettime,
 	[ __NR_clock_getres ] = (syscall_handler_t *) sys_clock_getres,
 	[ __NR_clock_nanosleep ] = (syscall_handler_t *) sys_clock_nanosleep,
-	[ __NR_statfs64 ] = (syscall_handler_t *) sys_statfs64,
-	[ __NR_fstatfs64 ] = (syscall_handler_t *) sys_fstatfs64,
 	[ __NR_tgkill ] = (syscall_handler_t *) sys_tgkill,
 	[ __NR_utimes ] = (syscall_handler_t *) sys_utimes,
-	[ __NR_fadvise64_64 ] = (syscall_handler_t *) sys_fadvise64_64,
-	[ __NR_vserver ] = (syscall_handler_t *) sys_vserver,
+	[ __NR_fadvise64 ] = (syscall_handler_t *) sys_fadvise64,
+	[ __NR_vserver ] = (syscall_handler_t *) sys_ni_syscall,
 	[ __NR_mbind ] = (syscall_handler_t *) sys_mbind,
 	[ __NR_get_mempolicy ] = (syscall_handler_t *) sys_get_mempolicy,
 	[ __NR_set_mempolicy ] = (syscall_handler_t *) sys_set_mempolicy,

@@ -267,9 +265,8 @@ syscall_handler_t *sys_call_table[] = {
 	[ __NR_mq_timedreceive ] = (syscall_handler_t *) sys_mq_timedreceive,
 	[ __NR_mq_notify ] = (syscall_handler_t *) sys_mq_notify,
 	[ __NR_mq_getsetattr ] = (syscall_handler_t *) sys_mq_getsetattr,
-	[ __NR_sys_kexec_load ] = (syscall_handler_t *) sys_ni_syscall,
+	[ __NR_kexec_load ] = (syscall_handler_t *) sys_ni_syscall,
 	[ __NR_waitid ] = (syscall_handler_t *) sys_waitid,
-	[ 285 ] = (syscall_handler_t *) sys_ni_syscall,
 	[ __NR_add_key ] = (syscall_handler_t *) sys_add_key,
 	[ __NR_request_key ] = (syscall_handler_t *) sys_request_key,
 	[ __NR_keyctl ] = (syscall_handler_t *) sys_keyctl,

_

Your code is at best redundant. And I'd bet beer that it is not what its author intended to write. So the bug is in GCC 3, imo.

Jörn

The cost of changing business rules is much more expensive for software
than for a secretaty.
-- unknown

On a binary distro they can recompile, too. Althoughg i'll admit it's not something a newbie should do.

It may be a reason for folks who do a lot of development. But that can hardly serve as a "main" reason. Speed of compilation, after all, is a one-time thing. Howver, as i've understood, there are more pressing reasons.

Ok. And a couple of hacks down the road, and the code will look nice and cryptic and a newbie trying to understand what's going on, will have a nice set of puzzles to solve.

Perhaps. But i think you'll agree it's not "The way to go".

From a strict ISO-C point of view, both are. It's a gcc-specific "feature" which (agreed) does come in handy sometimes. However it makes it quite hard to say which is the buggy version, since the "appropriate" behavior is a question of definition (by the gcc-folks). They may even argue that, having changed their minds about it, neither is buggy, but both conform to the specifications (for that specific functionality).

That's pretty much the trouble with relying on gcc-extensions: since there's no standard, it's difficult to tell what's wrong and what's right. I'll dive into it.

Regards,

Renate Meijer.

timeo hominem unius libri

Thomas van Aquino

Well, for "range" assignments GCC mustn't complain, but for the rest the double assignment laziness is not very useful. Could they at least add a -Wsomething inside -Wall or -W for this problem?

Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729
http://www.user-mode-linux.org/~blaisorblade

Hm, sorry about the wrong subject, there were only 8 patches.

And here's a diffstat of all of them, just to make this email worth reading and not just an apology:

 lib/rwsem-spinlock.c          |   42 ++++++++++++++++++++++++++----------------
 lib/rwsem.c                   |   16 ++++++++++------
 net/ipv4/xfrm4_output.c       |   12 ++++++------
 net/ipv6/xfrm6_output.c       |   12 ++++++------
 arch/um/kernel/skas/uaccess.c |    3 ++-
 arch/ia64/kernel/fsys.S       |    4 +++-
 arch/ia64/kernel/signal.c     |    3 ++-
 net/ipv4/tcp_input.c          |    5 ++++-
 fs/jbd/transaction.c          |    6 +++---
 drivers/i2c/chips/eeprom.c    |    3 ++-
 sound/core/timer.c            |    5 ++++-
 11 files changed, 68 insertions(+), 43 deletions(-)

thanks,

greg k-h