netdev - Re: NIU driver: Sun x8 Express Quad Gigabit Ethernet Adapter

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ada8wromu0g.fsf@cisco.com>
Date:	Wed, 12 Nov 2008 14:58:23 -0800
From:	Roland Dreier <rdreier@...co.com>
To:	David Miller <davem@...emloft.net>
Cc:	bhutchings@...arflare.com, jdb@...x.dk, netdev@...r.kernel.org
Subject: Re: NIU driver: Sun x8 Express Quad Gigabit Ethernet Adapter

 > Just google "C order of evaluation" and you will get hundreds
 > of tables, and all of them will have an entry for "|" (not
 > just "||") which says that operands are evaluated left to
 > right.

You're talking about associativity, which says how an expression like
"a | b | c" is implicitly parenthesized.  The order of evaluation is
undefined -- in fact the C standard I have says:

  Except as specified later (for the function-call (), &&, ||, ?:, and
  comma operators), the order of evaluation of subexpressions and the
  order in which side effects take place are both unspecified.

So there is no rule about which subexpression is evaluated first in an
expression like "a | b".

 > And since these MMIO reads are volatile operations, there is
 > no way the compiler can execute them out of order.

"volatile" just means that accessing a volatile expression is considered
a side effect -- and side effects are only ordered with respect to
sequence points.

So according to my understanding of the C standard, there is no required
on which readl() is done first in an expression like "readl(a) | readl(b)".

 > And the plain truth is that no compiler does, and that is what
 > matters in the end.

I think it's cleaner to avoid relying on undefined behavior (eg gcc 4.5
will probably break things), especially when the fix is so simple --
something the following should work fine:

diff --git a/drivers/net/niu.c b/drivers/net/niu.c
index 9acb5d7..1fb0d2f 100644
--- a/drivers/net/niu.c
+++ b/drivers/net/niu.c
@@ -51,8 +51,8 @@ MODULE_VERSION(DRV_MODULE_VERSION);
 #ifndef readq
 static u64 readq(void __iomem *reg)
 {
-	return (((u64)readl(reg + 0x4UL) << 32) |
-		(u64)readl(reg));
+	u64 v = readl(reg);
+	return v | (u64) readl(reg + 0x4UL) << 32;
 }

 static void writeq(u64 val, void __iomem *reg)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html