Re: TKIP encryption should allocate enough tailroom
Brandon Craig Rhodes wrote:
+ if (skb_tailroom(skb) 4) {
+ int err;
+ err = skb_padto(skb, skb-len + 4);
+ if (unlikely(err || skb_tailroom(skb) 4)) {
+ printk(KERN_DEBUG Failed to increase tailroom
+ for TKIP encrypt);
+ return err || -1;
The || operator in C doesn't act the same way it does in perl and ruby.
You're always returning 1 here.
+ return err || -1;
...and here as well.
-Mitch
-
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
Re: ATM bug found
(trimmed cc:'s since, IMO, isn't really all that general interest) Jeff Garzik wrote: drivers/atm/zatm.c: In function ?zatm_open?: drivers/atm/zatm.c:919: warning: ?pcr? may be used uninitialized in this function Yeah, looks like a bug. Not very high-impact because: 1. it only results in garbled data in vcc-qos.txtp which doesn't really mean much for UBR vcc's anyway 2. zatm is a more-obscure-than-obscure piece of hardware. I'm sure 3c501.c has more users these days :-) The fix is for alloc_shaper() should really do *pcr = ATM_MAX_PCR in the if (ubr) stanza. Chas, want to submit that in the next batch of patches? -Mitch - 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
Re: ATM firestream bug
Jeff Garzik wrote: 1) not safe on 64-bit Almost certainly correct. Probably never will be -- IIRC this SAR was mainly used in embedded apps. I don't know if any commercially-available PCI cards were ever made with it. I could be wrong though, it's been awhile since I was up on the ATM industry. 2) variable 'tmc0' is indeed potentially used uninit'd, in particular if make_rate() returns an error (use occurs before error check). Exact same error as the one you spotted in ambassador.c -- make_rate()'s error is not checked at all. It should be. -Mitch - 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
Re: ATM bug found
chas williams - CONTRACTOR wrote: still generates a warning from gcc though. The warning is bogus in this case, though -- the only way for *pcr to be unset is when alloc_shaper() returns non-zero + *pcr = 0; You're right, 0 is better than ATM_MAX_PCR here. -Mitch - 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
Re: [PATCH] bcm43xx: iw_priv_args names should be 16 characters
Erik Mouw wrote:
On Thu, Apr 13, 2006 at 10:29:19AM -0700, Stephen Hemminger wrote:
Shouldn't compiler have gagged on this?
Apparently not. At least the compiler I use doesn't warn about it (gcc
version 3.3.5 (Debian 1:3.3.5-13)).
Linus, this might be be something for sparse to check:
struct mystruct {
char name[16];
};
mystruct ms = { .name = muchlongerthan16characters };
If you actually try that example you'd see that gcc (even really old
versions) will warn you about that code.
What gcc WON'T warn about is if everything but the final '\0' fits in
the array.
So the code:
struct mystruct { char name[16]; };
struct mystruct exact_length = { .name = 0123456789abcdef };
struct mystruct one_too_long = { .name = 0123456789abcdefg };
Produces:
a.c:3: warning: initializer-string for array of chars is too long
a.c:3: warning: (near initialization for 'one_too_long.name')
I believe this is intentional and in line with the C specs. It's also
useful since it allows you to define macros like:
#define DECLARE_NON_TERMINATED_STRING(name, val) \
const name[sizeof(val ) - 1] = val;
Making sparse a bit stricter than gcc in this case might not be a bad idea,
though.
-Mitch
-
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
Re: Van Jacobson net channels
Jeff Garzik wrote: Once packets classified to be delivered to a specific local host socket, what further operations are require privs? What received packet data cannot be exposed to userspace? You just need to make sure that you don't leak data from other peoples sockets. Two issues I see: 1. If the card receives a long frame for application #1 and then receives a short frame for application #2, then you need to make sure that the data gets zeroed out first. So you need to limit this to only maximum-sized packets (or packets whose previous use was on the same flow). Probably not a big deal, since that's the performance-critical case anyway 2. More concerning is how you control what packets the app can see. If you made the memory frames all PAGE_SIZE then you could just give the app the packets to its flows by doing MMU tricks, but wouldn't that murder performance anyway? So I think the only real solution would be to allow the app to map all of the frames all of the time. So I agree that this would have to be CAP_NET_ADMIN only. -Mitch - 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
Re: [RFC][PATCH 3/3] TCP/IP Critical socket communication mechanism
Alan Cox wrote: But your user space that would add the routes is not so protected so I'm not sure this is actually a solution, more of an extended fudge. Yes, there's no 100% solution -- no matter how much memory you reserve and how many paths you protect if you try hard enough you can come up with cases where it'll fail. (I'm swapping to NFS across a tun/tap interface to a custom userland SSL tunnel to a server across a BGP route...) However, if the 'extended fundge' pushes a problem from can happen, even in a very normal setup territory to only happens if you're doing something pretty weird then is it really such a bad thing? I think the cost in code complexity looks pretty reasonable. +#define SK_CRIT_ALLOC(sk, flags) ((sk-sk_allocation __GFP_CRITICAL) | flags) Lots of hidden conditional logic on critical paths. How expensive is it compared to the allocation itself? +#define CRIT_ALLOC(flags) (__GFP_CRITICAL | flags) Pointless obfuscation Fully agree. -Mitch - 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
Re: [DCCP]: Fix compiler warnings
Patrick McHardy wrote: -static inline const int before48(const u64 seq1, const u64 seq2) +static inline int before48(const u64 seq1, const u64 seq2) Perhaps __attribute__ ((const)) is what was meant here? -Mitch - 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
Re: [PATCH] TCP Offload (TOE) - Chelsio
The networking gurus can comment on the internals of your patch better than
I can. Just a few style notes though:
+#ifdef CONFIG_TCP_OFFLOAD
+#define NETIF_F_TCPIP_OFFLOAD65536 /* Can offload TCP/IP */
+#endif
No need to protect this inside CONFIG_* option
+/* TOE API */
+#ifdef CONFIG_TCP_OFFLOAD
+#define TCPDIAG_OFFLOAD 5
+#endif
Ditto
+#ifdef CONFIG_TCP_OFFLOAD
+struct toe_funcs;
+#endif
Ditto
+#ifdef CONFIG_TCP_OFFLOAD
+#include linux/toedev.h
+#endif
Include linux/toedev.h unconditionally. Have it handle the !CONFIG_TCP_OFFLOAD
case itself by declaring noop macros for things like toe_neigh_update().
This way you can remove a lot of the #ifdef's you've sprinkled all over the
.c files
+#define boot_phase 0
Some explaination here? It looks like something left over from development.
+#ifndef __raise_softirq_irqoff
+#define __raise_softirq_irqoff(nr) __cpu_raise_softirq(smp_processor_id(),
nr)
+#endif
What is this needed for?
+static int toedev_init(void);
This forward declaration seems to be only needed for the boot_phase thing
above, so if that goes this can go as well.
+/*
+ * Allocate a unique index for a TOE device. We keep the index within 30
bits
Maybe look at lib/idr.c to handle this?
+ struct toedev *dev = kmalloc(sizeof(struct toedev), GFP_KERNEL);
+
+ if (dev) {
+ memset(dev, 0, sizeof(struct toedev));
Minor nitpick (that some might disagree with)... I usually prefer:
struct toedev *dev = kmalloc(sizeof(*dev), GFP_KERNEL);
+int toe_receive_skb(struct toedev *dev, struct sk_buff **skb, int n)
+{
+ int i;
n and i should probably be unsigned int
+#ifdef CONFIG_TCP_OFFLOAD
+ tcp_listen_offload(sk);
+#endif
Another example of something that could be an empty macro in a .h file for
the !CONFIG_TCP_OFFLOAD case.
+#ifndef CONFIG_TCP_OFFLOAD
+static
+#endif
Don't do this... just make it non-static unconditionally. It's not worth
the ugliness. Same applies to other places.
+#ifndef CONFIG_TCP_OFFLOAD
+static
+#endif
+__inline__ void __tcp_inherit_port(struct sock *sk, struct sock *child)
{
struct tcp_bind_hashbucket *head =
tcp_bhash[tcp_bhashfn(inet_sk(child)-num)];
@@ -351,7 +357,10 @@
}
}
Things that are inline and are now going to be shared really need to just
remain static inline and move to a header file probably
+#ifdef CONFIG_TCP_OFFLOAD
+ if (tcp_connect_offload(sk))
+ return 0;
+#endif
Just another example of the kind of #ifdef that doesn't belong in the .c
files. If the !CONFIG_TCP_OFFLOAD case just had
#define tcp_connect_offload(sk) (0)
then you can skip the #ifdef
+#ifndef CONFIG_TCP_OFFLOAD
LIMIT_NETDEBUG(printk(KERN_DEBUG TCP: drop open
request from %u.%u.
%u.%u/%u\n,
NIPQUAD(saddr),
ntohs(skb-h.th-source)));
+#else
+ NETDEBUG(if (net_ratelimit()) \
+ printk(KERN_DEBUG TCP: drop open
+request from %u.%u.
+%u.%u/%u\n, \
+NIPQUAD(saddr),
+ntohs(skb-h.th-source)));
+#endif
Huh? What about TOE requires changes to printk ratelimiting?
-Mitch
-
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
Re: [PATCH] TCP Offload (TOE) - Chelsio
I'm fairly pessimistic about full TOE also, I just want to see the patch cleaned up a bit so we can see the exact impact it would have. The RX optimization work presented in the Neterion and Intel papers at OLS sounds a lot more interesting to me though. However, I do want to comment on one statement of yours: Jeff Garzik wrote: 3) Netfilter. Either a TOE NIC (a) doesn't support netfilter, (b) needs far-reaching packet mangling hooks, or (c) includes its own custom netfilter [clone], with attendant bugs and maintenance issues. I don't think netfilter is a big deal. The kernel could still check the TCP handshake packets (or, if needed, faked-up versions with the same data) at accept()/connect() time. If those pass muster it's a pretty good bet that the other 100,000 packets making up that TCP connection would also. Of course this limitation would need to be documented but I doubt most netfilter users would mind too much. There's obviously edge cases where you can lose like if you update the netfilter rules you ideally want to revalidate all the currently open connections. Since TOE hardware is designed to help the TCP end point you probably don't have to worry about NAT or other fancy mangling on these interfaces. -Mitch - 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
