BTW, why does csum_tcpudp_nofold() have such a prototype?
It takes IP addresses as unsigned long and proto as unsigned short;
the former is bloody odd on 64bit boxen and the latter is bloody
odd, period. The value we are interested in is 8bit; all callers
pass either an explicit constant smaller than 256 or iphdr ->protocol
(__u8). Moreover, in any arithmetics both __u8 and __u16 will be
promoted to the same thing, so even arguments about avoiding casts
in the function body do not apply...
Even funnier, prototype depends on target; amd64 csum_tcpudp_nofold()
takes unsigned saddr, unsigned daddr; alpha has both unsigned long...
At the same time, amd64 has
static inline unsigned short int
csum_tcpudp_magic(unsigned long saddr, unsigned long daddr,
unsigned short len, unsigned short proto, unsigned int sum)
{
return csum_fold(csum_tcpudp_nofold(saddr,daddr,len,proto,sum));
}
so we pass 32bit value to that puppy as 64bit argument, only to cut it
back to 32bit when we pass it to csum_tcpudp_nofold()...
That's one hell of a hot path, so I'd rather avoid messing with
it without a very good idea of what's going on. One guaranteed-to-be-safe
way of annotating it is
#define csum_tcp_magic(saddr, daddr, len, proto, sum) \
csum_tcp_magic((__force u32)(__be32)saddr, (__force u32)(__be32)daddr,\
len, proto, sum)
which would verify that arguments have the right types and have those casts
leave the generated code as-is - actual arguments _are_ unsigned int from
C point of view, so those casts will leave arguments as-is.
Another issue is more subtle; what should be returned by csum_fold()
and friends? Answers are split between unsigned short and unsigned int;
that wouldn't matter, but if we make it __be16, we'll immediately run into
major mess with sparse.
The thing is, for smaller-than-int bitwise types (e.g. __be16)
~ is a prohibited operation. The reason is simple: integer promotion
happens before ~, so we are guaranteed that value of ~x will _not_
be within range of our type. I can try to teach sparse about such
"slightly fouled" __be16, so that conversion back to our type (e.g.
from assignment) would go without complaints, but there are some limits
to that.
[following is for people familiar with sparse internals; everybody else
can safely skip it]
What I propose is a new node type: SYM_FOULED. It would be
similar to SYM_RESTRICTED and base type always would be some smaller-than-int
restricted type. Rules:
* ~small_restricted => corresponding fouled
* any arithmetics that would be banned for restricted => same as if
we would have restricted
* if t1 is restricted type and t2 - its fouled analog, then
t1 & t2 => t1, t1 | t2 => t2, t1 ^ t2 => t2.
* conversion of t2 to t1 is silent (be it passing as argument
or assignment). Anything else is banned.
* x ? t1 : t2 => t2
* ~t2 => t2 (_not_ t1; something like ~(x ? y : ~y) is still fouled)
* x ? t2 : t2 => t2, t2 {&,|,^} t2 => t2 (yes, even ^ - same as before).
* x ? t2 : constant_valid_for_t1 => t2
* !t2 => warning, ditto for comparisons involving t2 in any way.
* wrt casts t2 acts exactly as t1 would.
* for sizeof, typeof and alignof t2 acts as promoted t1. Note that
fouled can never be an lvalue or have types derived from it - can't happen.
Objections? Basically, from C POV any fouled value is int or unsigned int
and we avoid generating a warning only if its upper bits will eventually
be discarded. Rules above guarantee that, AFAICS. I can implement that
without too much PITA, if nobody objects. Linus?
[here endeth the sparse-related part]
Note that some places around checksum handling *WILL* give complaints,
no matter how smart sparse might become. When we stuff big-endian
16bit values or ~ of such values into u32 array and calculate checksum
of that, we rely on the fact that both all-0 and all-1 16bit words _and_
any reordering of words do not affect the checksum, which is far beyond
anything sparse could be expected to understand. For places like that
(mostly in netfilter code) a forced cast and comment explaining what's
going on and what we are relying upon would be the right thing, IMO.
No need to reproduce the entire RFC1071, but reference to it would not be
a bad thing either...
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
