* Re: udp.c
From: Andi Kleen @ 2005-06-14 0:12 UTC (permalink / raw)
To: David S. Miller; +Cc: jesper.juhl, mru, rommer, bernd, linux-kernel, netdev
In-Reply-To: <20050613.170045.74749212.davem@davemloft.net>
On Mon, Jun 13, 2005 at 05:00:45PM -0700, David S. Miller wrote:
> From: Andi Kleen <ak@muc.de>
> Date: Tue, 14 Jun 2005 01:53:56 +0200
>
> > Still does, as does x86-64, but actually it could be changed now using
> > change_page_attr (and then later undoing it). Is it worth it?
>
> A lot of bootup OOPS's have occured in the past and
> were never discovered until someone on a non-x86
> platform tested the patch.
Ok I can fix it, but only reasonably after mem_init (at least without
hacking up change_page_attr a lot to deal with bootmem).
Is that still worth it?
-Andi
^ permalink raw reply
* Re: udp.c
From: David S. Miller @ 2005-06-14 1:11 UTC (permalink / raw)
To: ak; +Cc: jesper.juhl, mru, rommer, bernd, linux-kernel, netdev
In-Reply-To: <20050614001249.GF86745@muc.de>
From: Andi Kleen <ak@muc.de>
Date: 14 Jun 2005 02:12:49 +0200,Tue, 14 Jun 2005 02:12:49 +0200
> Ok I can fix it, but only reasonably after mem_init (at least without
> hacking up change_page_attr a lot to deal with bootmem).
> Is that still worth it?
I think so.
^ permalink raw reply
* [PATCH] tg3_msi() and weakly ordered memory
From: Grant Grundler @ 2005-06-14 3:37 UTC (permalink / raw)
To: David S. Miller; +Cc: mchan, netdev, iod00d
Dave,
I reviewed the "new" (to me) use of tags and MSI in tg3_msi() and
tg3_poll() and I like the new scheme. It's pretty clean.
But I did come up with four potential "issues" - mostly revolving
around enforcing order of memory access on weakly ordered platforms:
1) tg3_poll() and tg3_msi() are not consistent with use of rmb().
tg3_poll has an rmb() between reading status_tag and tg3_has_work().
The patch (against tg3 v3.29) below adds a similar rmb() to tg3_msi().
Does tg3_msi() need a "rmb()" like in the attached patch?
Or rather a mb() to deal with clearing SD_STATUS_UPDATED bit?
2) tg3_poll() and tg3_msi() are not consistent on how they clear
the SD_STATUS_UPDATED bit. tg3_poll() does not clear SD_STATUS_UPDATED
bit after reading status_tag. I think everytime the driver discovers
the status_tag changed, it should to clear SD_STATUS_UPDATED.
Michael, can you confirm/deny that offhand?
I'm not sure anymore what order the sblk fields (status_tag, tx_consumer,
and rx_producer) should be read before clearing SD_STATUS_UPDATED bit.
I expect a recommended order exists.
ISTR something like:
read status_tag
rmb()
read tx_consumer and rx_producer
mb()
clear SD_STATUS_UPDATED
3) Based on the above sequence, tg3 might need one more rmb() between
reading sblk status_tag and the inline code for tg3_has_work().
4) I'd also prefer if tg3 would read tx_consumer/rx_producer fields
*only* in tg3_msi() and tg3_poll() when sblk status_tag is read.
All other references (e.g. tg3_has_work(), tg3_rx(), etc) would use
a cached copy of those fields.
My goal would be to reduce the competition for access to sblk
cacheline and get the memory ordering issues right.
My fear is regularly reading the cacheline by the CPU will take
away exclusive (write) access from the IO subsystem and ping-pong
the cacheline more often than necessary.
Would you entertain a patch for this?
thanks,
grant
Signed-off-by: Grant Grundler <iodood@hp.com>
--- a/drivers/net/tg3.c 25 May 2005 17:12:47 -0000 1.35
+++ b/drivers/net/tg3.c 14 Jun 2005 01:37:43 -0000
@@ -2946,6 +2946,7 @@ static irqreturn_t tg3_msi(int irq, void
*/
tw32_mailbox(MAILBOX_INTERRUPT_0 + TG3_64BIT_REG_LOW, 0x00000001);
tp->last_tag = sblk->status_tag;
+ rmb():
sblk->status &= ~SD_STATUS_UPDATED;
if (likely(tg3_has_work(tp)))
netif_rx_schedule(dev); /* schedule NAPI poll */
^ permalink raw reply
* Re: [PATCH 2.4.31] e1000: unlock tx_lock before return
From: Michael Renzmann @ 2005-06-14 6:51 UTC (permalink / raw)
To: netdev; +Cc: linux-kernel
In-Reply-To: <42ADE90A.8080804@otaku42.de>
[-- Attachment #1: Type: text/plain, Size: 712 bytes --]
Hi all.
Sorry for replying to my own post. I rediffed the patch to make it a -p1
patch as described by DaveM in another (netdev) thread. Also CC'd lkml.
lkml'ers, please CC replies to me, since I'm not subscribed.
Michael Renzmann wrote:
> It may happen that the e1000 driver returns from e1000_xmit_frame()
> while still holding the tx_lock spinlock. This is at least true for the
> version that comes with kernel 2.4.31. The problem doesn't exist in
> 2.6.11.12.
>
> The attached patch applies to vanilla 2.4.31. I couldn't test the patch
> myself (no e1000 available), but it's a trivial patch and thus it's
> expected to work without problems.
>
> Signed-off-by: Michael Renzmann <mrenzmann@otaku42.de>
[-- Attachment #2: e1000-2.4.31-unlock_before_return.patch --]
[-- Type: text/x-patch, Size: 407 bytes --]
--- a/drivers/net/e1000/e1000_main.c 2005-04-04 03:42:19.000000000 +0200
+++ b/drivers/net/e1000/e1000_main.c 2005-06-13 21:39:58.000000000 +0200
@@ -1955,8 +1955,8 @@
if(unlikely(e1000_82547_fifo_workaround(adapter, skb))) {
netif_stop_queue(netdev);
mod_timer(&adapter->tx_fifo_stall_timer, jiffies);
- return 1;
spin_unlock_irqrestore(&adapter->tx_lock, flags);
+ return 1;
}
}
^ permalink raw reply
* Re: [PATCH] tg3_msi() and weakly ordered memory
From: Grant Grundler @ 2005-06-14 15:40 UTC (permalink / raw)
To: Michael Chan; +Cc: Grant Grundler, David S. Miller, netdev
In-Reply-To: <B1508D50A0692F42B217C22C02D84972067F0804@NT-IRVA-0741.brcm.ad.broadcom.com>
On Mon, Jun 13, 2005 at 11:46:47PM -0700, Michael Chan wrote:
> Yes, you're right. rmb() is needed between reading the tag and tg3_has_work()
> to guarantee strict ordering. Otherwise, tg3_has_work() may get ahead and
> read stale information that may be older than the tag.
Ok - thanks for confirming. I just wasn't sure any more.
It's been a year or so since I looked at this last time.
> But the clearing of
> the SD_STATUS_UPDATED bit does not need any additional barriers.
...
> You're right again. The SD_STATUS_UPDATED bit should be cleared right before
> checking for new work. Clearing the SD_STATUS_UPDATED bit tells the non-msi
> irq handler that all work up to the last status block update has been
> processed.
...
> The clearing of the SD_STATUS_UPDATED bit does not have to follow very strict
> ordering for the following reasons:
>
> 1. It has no hardware significance. It is purely to tell the irq handler that
> the current status block has been processed. For MSI, since we don't even
> check that bit in tg3_msi(), we can skip clearing that bit. But I think it is
> safer to clear it because tg3_cond_int() is checking it.
Ok - I thought the NIC was reading that back for some reason.
If we can ignore SD_STATUS_UPDATED and use a flag not related
to sblk, I think it would be cacheline friendlier. But it's
a minor issue.
> 2. We only clear it when interrupt from the NIC is disabled, either in irq
> handler or tg3_poll(). So there is no potential contention.
>
> So the current sequence is fine.
ok - thank you for the clarification.
> It is important to read the actual status block with the latest indices to
> determine whether there is new work, especially in the non-tagged case where
> you may have race condition between software and hardware.
Certainly...my point was we should not read them on every iteration
of the RX or TX loop that processes packets - but rather outside that loop.
And I'd think we want to read the three values (status tag, tx_consumer,
and rx_producer) as a set - ie read them and process stuff until
we've exhausted the "work quota" or the driver has caught up
to the HW (status tag stops changing).
I don't see a problem with exiting tg3_poll despite more work
pending if we know we are going to catch it on the next round.
After all, we are polling - latency is going to be semi random
depending on where we are in the sequence anyway.
thanks,
grant
^ permalink raw reply
* Re: [PATCH] tg3_msi() and weakly ordered memory
From: Grant Grundler @ 2005-06-14 15:46 UTC (permalink / raw)
To: Michael Chan; +Cc: David S. Miller, iod00d, netdev
In-Reply-To: <B1508D50A0692F42B217C22C02D84972067F0805@NT-IRVA-0741.brcm.ad.broadcom.com>
On Mon, Jun 13, 2005 at 11:54:23PM -0700, Michael Chan wrote:
> > Once you write "0x1" to the mailbox register, the device stops
> > updating the status block and stops generating interrupts.
> >
> > That is what makes a lot of things safe.
>
> Only interrupts are stopped, status block will still be updated subject to
> during-ints coalescing.
Will setting during-ints to a very high threshhold essentially allow
us to "indefinitely" process stuff without taking any interrupts?
Would the threshhold counter get reset every time we write back
the status tag WITHOUT re-enableing interrupts?
If not, I suspect the CPU will circulate in tg3_poll until during-ints
is exhausted and DMA will stop until CPU reenables interrupts.
ie not until it's done processing outstanding packets.
thanks,
grant
^ permalink raw reply
* Re: [PATCH] net: fix sparse warning (plain int as NULL)
From: Jesper Juhl @ 2005-06-14 16:39 UTC (permalink / raw)
To: David S. Miller; +Cc: juhl-lkml, linux-kernel, ross.biro, netdev
In-Reply-To: <20050613.143114.35469427.davem@davemloft.net>
On 6/13/05, David S. Miller <davem@davemloft.net> wrote:
> From: Jesper Juhl <jesper.juhl@gmail.com>
> Date: Mon, 13 Jun 2005 23:21:56 +0200
>
> > Since tcp_ack_saw_tstamp() in 2.6.12-rc6-git6 only takes two arguments
> > this patch is only relevant for -mm.
>
> Thanks for the clarification.
>
You are welcome. Should I just send the patch on to Andrew for inclusion, or?
--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
^ permalink raw reply
* Re: [PATCH] tg3_msi() and weakly ordered memory
From: Grant Grundler @ 2005-06-14 17:55 UTC (permalink / raw)
To: Michael Chan; +Cc: Grant Grundler, David S. Miller, netdev
In-Reply-To: <B1508D50A0692F42B217C22C02D84972067F0804@NT-IRVA-0741.brcm.ad.broadcom.com>
On Mon, Jun 13, 2005 at 11:46:47PM -0700, Michael Chan wrote:
> rmb() is needed between reading the tag and tg3_has_work()
> to guarantee strict ordering.
Thinking about this more... tg3_has_work() could be reduced to
comparing status tag with last_tag (vs each of the TX/RX indices).
That assumes all the tg3 NICs support status tags...if not, then we
have to keep checking indices.
[ BTW, I noticed spin_lock*(&tp->lock) calls are nested in tg3_poll.
That's a bug, right? I'm still looking at v3.29 ]
The current implementation of tg3_poll() processes TX and then RX.
The status tag we read afterwards and the TX/RX indices checked
could be newer than the TX/RX indices used during processing.
Is tg3 then roughly rate limited to the TX and RX queue depth
per poll interval?
(I'm still thinking during-ints limits how much DMA can occur)
Given TG3_TX_RING_SIZE is 512, then I would max out at ~500Kpps
if there is any RX traffic that causes tg3_has_work() to come
back true. While this might be normally ok, I'm looking to
maximize pktgen output w/o disabling/enabling interrupts
for each "batch" of TX packets.
> > 2) tg3_poll() and tg3_msi() are not consistent on how they clear
> > the SD_STATUS_UPDATED bit. tg3_poll() does not clear SD_STATUS_UPDATED
> > bit after reading status_tag. I think everytime the driver discovers
> > the status_tag changed, it should to clear SD_STATUS_UPDATED.
> > Michael, can you confirm/deny that offhand?
>
> You're right again. The SD_STATUS_UPDATED bit should be cleared right before
> checking for new work. Clearing the SD_STATUS_UPDATED bit tells the non-msi
> irq handler that all work up to the last status block update has been
> processed.
If I understood this correctly, tg3 may already have new work pending
when tg3_has_work() is called from tg3_poll(). tg3_poll() does not tell
the card anything but promises to pick up where it left off the
next time tg3_poll() is called. If we don't tell the card anything,
it means at some point it's going to stop doing DMA....this might be
one of the things preventing tg3 from doing link rate with pktgen
pushing 64byte packets.
...
> It is important to read the actual status block with the latest indices to
> determine whether there is new work, especially in the non-tagged case where
> you may have race condition between software and hardware.
Yes - I think I understand were several of the races can occur.
Probably not seeing all of them though.
thanks again,
grant
^ permalink raw reply
* Re: [PATCH] tg3_msi() and weakly ordered memory
From: Grant Grundler @ 2005-06-14 18:04 UTC (permalink / raw)
To: Michael Chan; +Cc: Grant Grundler, David S. Miller, netdev
In-Reply-To: <1118767397.7059.19.camel@rh4>
On Tue, Jun 14, 2005 at 09:43:17AM -0700, Michael Chan wrote:
...
> Something like:
>
> if (sblk->status_tag != tp->last_tag)
> clear_interrupt();
> netif_rx_schedule();
>
> This way we don't have to clear the SD_STATUS_UPDATED bit. I will
> experiment with this and see if it works well.
that sounds good - thanks.
> I don't think we are reading the index on every iteration. In tg3_rx(),
> we read it at the beginning before the loop, and one more time if we
> have caught up with the hw index before exiting the loop.
oh - sorry - my bad. Same is true for tg3_tx().
And I just noticed I'm smoking crack on "nested locks" too...
one is "lock" and the other is "tx_lock".
*sigh* - need more sleep.
> I mildly disagree. I think we should maximize the amount of work done in
> tg3_poll(). For example, reading the rx_producer index one more time
> when we have caught up with hw index before exiting the loop is a good
> thing IMO.
ok.
thanks,
grant
^ permalink raw reply
* Netdev List (Re: [PATCH] Ensure to use icmpv6_socket in non-preemptive context.)
From: YOSHIFUJI Hideaki / 吉藤英明 @ 2005-06-15 0:53 UTC (permalink / raw)
To: davem; +Cc: netdev, netdev
In-Reply-To: <20050613.150037.70219957.davem@davemloft.net>
In article <20050613.150037.70219957.davem@davemloft.net> (at Mon, 13 Jun 2005 15:00:37 -0700 (PDT)), "David S. Miller" <davem@davemloft.net> says:
> Please update your address book, netdev has moved to vger.kernel.org
> :-)
Oh, I didn't noticed that;
Sorry, I could have missed the announcement.
Do we have to subscribe new list by ourselves?
--yoshfuji
^ permalink raw reply
* Re: source routing - bug?
From: Thomas Graf @ 2005-06-15 1:05 UTC (permalink / raw)
To: Tim Newsham; +Cc: linux-net, netdev
In-Reply-To: <Pine.BSI.4.61.0506061455340.11929@malasada.lava.net>
* Tim Newsham <Pine.BSI.4.61.0506061455340.11929@malasada.lava.net> 2005-06-06 15:02
> Using the netcat utility I run tests from machine A:
>
> $ nc -g B C 3333
> I see packets forwarded to B then on to C and the connection
> completes successfully.
>
> $ nc -g A -g B C 3333
> I see packets forwarded over lo to A, and no further packets
> are sent over lo or eth0. Machines B and C receive no packets.
A 127.0.0.1 source address is considered martian by the source
validation code as proposed by rfc1812. You should see warnings
in the console if you have martian logging enabled.
> $ nc -g B A 3333
> I see packets forwarded out over eth0 to B and then back to A
> but machine A never seems to accept the packets or reply with
> a SYN or RST.
A will do a route lookup of [src=A dst=A] which results in your
local route being returned. Again this violates rfc1812 because
that route will not be a unicast route but a local one. A martian
source warning should be logged if enabled.
^ permalink raw reply
* Re: source routing - bug?
From: Tim Newsham @ 2005-06-15 1:15 UTC (permalink / raw)
To: Thomas Graf; +Cc: linux-net, netdev
In-Reply-To: <20050615010548.GE22463@postel.suug.ch>
>> $ nc -g A -g B C 3333
>> I see packets forwarded over lo to A, and no further packets
>> are sent over lo or eth0. Machines B and C receive no packets.
>
> A 127.0.0.1 source address is considered martian by the source
> validation code as proposed by rfc1812. You should see warnings
> in the console if you have martian logging enabled.
I see the same behavior when the source address is explicitely bound
to an address belonging to a local NIC (-s option to netcat).
>> $ nc -g B A 3333
>> I see packets forwarded out over eth0 to B and then back to A
>> but machine A never seems to accept the packets or reply with
>> a SYN or RST.
>
> A will do a route lookup of [src=A dst=A] which results in your
> local route being returned. Again this violates rfc1812 because
> that route will not be a unicast route but a local one. A martian
> source warning should be logged if enabled.
Is there any way to enable routing for this case? This is a
desirable source route at times. I would hate to have to boot
FreeBSD every time I want to use this feature.
Tim Newsham
http://www.lava.net/~newsham/
^ permalink raw reply
* Re: source routing - bug?
From: Thomas Graf @ 2005-06-15 1:24 UTC (permalink / raw)
To: Tim Newsham; +Cc: linux-net, netdev
In-Reply-To: <Pine.BSI.4.61.0506141513050.28194@malasada.lava.net>
* Tim Newsham <Pine.BSI.4.61.0506141513050.28194@malasada.lava.net> 2005-06-14 15:15
> >> $ nc -g A -g B C 3333
> >> I see packets forwarded over lo to A, and no further packets
> >> are sent over lo or eth0. Machines B and C receive no packets.
> >
> >A 127.0.0.1 source address is considered martian by the source
> >validation code as proposed by rfc1812. You should see warnings
> >in the console if you have martian logging enabled.
>
> I see the same behavior when the source address is explicitely bound
> to an address belonging to a local NIC (-s option to netcat).
Since your local address is not a unicast you'll run into the same
martian rule as below.
> >A will do a route lookup of [src=A dst=A] which results in your
> >local route being returned. Again this violates rfc1812 because
> >that route will not be a unicast route but a local one. A martian
> >source warning should be logged if enabled.
>
> Is there any way to enable routing for this case? This is a
> desirable source route at times. I would hate to have to boot
> FreeBSD every time I want to use this feature.
I can only think of one way which is to change fib_validate_source()
and have it accept RTN_LOCAL routes as well, i.e. change
if (res.type != RTN_UNICAST)
into
if (res.type != RTN_UNICAST && res.type != RTN_LOCAL)
I'm not sure if this breaks other code though.
^ permalink raw reply
* [-mm PATCH] signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 19:13 UTC (permalink / raw)
To: David S. Miller
Cc: Hideaki YOSHIFUJI, Alexey Kuznetsov, James Morris,
Fred N. van Kempen, Ross Biro, netdev, linux-kernel
This patch cleans up some signed versus unsigned variable use in
net/ipv4/raw.c
Before this patch, building net/ipv4/raw.c from 2.6.12-rc6-mm1 with
gcc -W produces a bunch of warnings :
net/ipv4/raw.c: In function `raw_send_hdrinc':
net/ipv4/raw.c:272: warning: comparison between signed and unsigned
net/ipv4/raw.c:301: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_probe_proto_opt':
net/ipv4/raw.c:340: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_sendmsg':
net/ipv4/raw.c:387: warning: comparison of unsigned expression < 0 is always false
net/ipv4/raw.c:405: warning: comparison between signed and unsigned
net/ipv4/raw.c:517: warning: signed and unsigned type in conditional expression
net/ipv4/raw.c:374: warning: unused parameter `iocb'
net/ipv4/raw.c: In function `raw_close':
net/ipv4/raw.c:527: warning: unused parameter `timeout'
net/ipv4/raw.c: In function `raw_bind':
net/ipv4/raw.c:545: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_recvmsg':
net/ipv4/raw.c:613: warning: signed and unsigned type in conditional expression
net/ipv4/raw.c:565: warning: unused parameter `iocb'
net/ipv4/raw.c: In function `raw_seticmpfilter':
net/ipv4/raw.c:627: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_geticmpfilter':
net/ipv4/raw.c:643: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_seq_stop':
net/ipv4/raw.c:799: warning: unused parameter `seq'
net/ipv4/raw.c:799: warning: unused parameter `v'
net/ipv4/raw.c: In function `raw_seq_open':
net/ipv4/raw.c:847: warning: unused parameter `inode'
10 of which are related to signedness issues.
With this patch we are down to just 4 signed vs unsigned warnings -
cleaning up the last 4 didn't really seem feasible.
net/ipv4/raw.c: In function `raw_sendmsg':
net/ipv4/raw.c:405: warning: comparison between signed and unsigned
net/ipv4/raw.c:374: warning: unused parameter `iocb'
net/ipv4/raw.c: In function `raw_close':
net/ipv4/raw.c:530: warning: unused parameter `timeout'
net/ipv4/raw.c: In function `raw_bind':
net/ipv4/raw.c:548: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_recvmsg':
net/ipv4/raw.c:568: warning: unused parameter `iocb'
net/ipv4/raw.c: In function `raw_seticmpfilter':
net/ipv4/raw.c:633: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_geticmpfilter':
net/ipv4/raw.c:649: warning: comparison between signed and unsigned
net/ipv4/raw.c: In function `raw_seq_stop':
net/ipv4/raw.c:805: warning: unused parameter `seq'
net/ipv4/raw.c:805: warning: unused parameter `v'
net/ipv4/raw.c: In function `raw_seq_open':
net/ipv4/raw.c:853: warning: unused parameter `inode'
So, here's the patch. I hope you like it and want to merge it :-)
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---
net/ipv4/raw.c | 18 ++++++++++++------
1 files changed, 12 insertions(+), 6 deletions(-)
--- linux-2.6.12-rc6-mm1-orig/net/ipv4/raw.c 2005-06-12 15:58:58.000000000 +0200
+++ linux-2.6.12-rc6-mm1/net/ipv4/raw.c 2005-06-15 20:55:06.000000000 +0200
@@ -259,7 +259,7 @@ int raw_rcv(struct sock *sk, struct sk_b
return 0;
}
-static int raw_send_hdrinc(struct sock *sk, void *from, int length,
+static int raw_send_hdrinc(struct sock *sk, void *from, size_t length,
struct rtable *rt,
unsigned int flags)
{
@@ -298,7 +298,7 @@ static int raw_send_hdrinc(struct sock *
goto error_fault;
/* We don't modify invalid header */
- if (length >= sizeof(*iph) && iph->ihl * 4 <= length) {
+ if (length >= sizeof(*iph) && (size_t)(iph->ihl * 4) <= length) {
if (!iph->saddr)
iph->saddr = rt->rt_src;
iph->check = 0;
@@ -332,7 +332,7 @@ static void raw_probe_proto_opt(struct f
u8 __user *type = NULL;
u8 __user *code = NULL;
int probed = 0;
- int i;
+ unsigned int i;
if (!msg->msg_iov)
return;
@@ -384,7 +384,7 @@ static int raw_sendmsg(struct kiocb *ioc
int err;
err = -EMSGSIZE;
- if (len < 0 || len > 0xFFFF)
+ if (len > 0xFFFF)
goto out;
/*
@@ -514,7 +514,10 @@ done:
kfree(ipc.opt);
ip_rt_put(rt);
-out: return err < 0 ? err : len;
+out:
+ if (err < 0)
+ return err;
+ return len;
do_confirm:
dst_confirm(&rt->u.dst);
@@ -610,7 +613,10 @@ static int raw_recvmsg(struct kiocb *ioc
copied = skb->len;
done:
skb_free_datagram(sk, skb);
-out: return err ? err : copied;
+out:
+ if (err)
+ return err;
+ return copied;
}
static int raw_init(struct sock *sk)
Please CC me on replies.
--
Jesper Juhl
^ permalink raw reply
* Re: [-mm PATCH] signed vs unsigned cleanup in net/ipv4/raw.c
From: David S. Miller @ 2005-06-15 19:16 UTC (permalink / raw)
To: juhl-lkml
Cc: yoshfuji, kuznet, jmorris, waltje, ross.biro, netdev,
linux-kernel
In-Reply-To: <Pine.LNX.4.62.0506152101350.3842@dragon.hyggekrogen.localhost>
I'm not merging this thing, at least no all at once.
"size_t" vs. "unsigned int" vs. "int" length comparisons are where all
the security problems come from in the protocol stack
Therefore you should make a seperate patch for each type
change you make and explain why it doesn't add some regression
in terms of signedness issues.
^ permalink raw reply
* Re: [-mm PATCH] signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 19:28 UTC (permalink / raw)
To: David S. Miller
Cc: juhl-lkml, yoshfuji, kuznet, jmorris, waltje, ross.biro, netdev,
linux-kernel
In-Reply-To: <20050615.121628.112622743.davem@davemloft.net>
On Wed, 15 Jun 2005, David S. Miller wrote:
>
> I'm not merging this thing, at least no all at once.
>
> "size_t" vs. "unsigned int" vs. "int" length comparisons are where all
> the security problems come from in the protocol stack
>
> Therefore you should make a seperate patch for each type
> change you make and explain why it doesn't add some regression
> in terms of signedness issues.
>
Fair enough, I'll split it into little bits and submit them one by one
with explanations. Not a problem at all.
--
Jesper Juhl
^ permalink raw reply
* Re: [-mm PATCH] signed vs unsigned cleanup in net/ipv4/raw.c
From: David S. Miller @ 2005-06-15 19:58 UTC (permalink / raw)
To: juhl-lkml
Cc: yoshfuji, kuznet, jmorris, waltje, ross.biro, netdev,
linux-kernel
In-Reply-To: <Pine.LNX.4.62.0506152127150.3842@dragon.hyggekrogen.localhost>
From: Jesper Juhl <juhl-lkml@dif.dk>
Date: Wed, 15 Jun 2005 21:28:23 +0200 (CEST)
> Fair enough, I'll split it into little bits and submit them one by one
> with explanations. Not a problem at all.
Thanks a lot Jesper.
^ permalink raw reply
* Re: TCP prequeue performance
From: David S. Miller @ 2005-06-15 20:51 UTC (permalink / raw)
To: cndougla; +Cc: linux-kernel, netdev
In-Reply-To: <BED5FA3B.2A0%cndougla@purdue.edu>
From: Chase Douglas <cndougla@purdue.edu>
Date: Wed, 15 Jun 2005 15:31:07 -0500
> Note the decreases in the system and real times. These numbers are fairly
> stable through 10 consecutive benchmarks of each. If I change message sizes
> and number of connections, the difference can narrow or widen, but usually
> the non-prequeue beats the prequeue with respect to system and real time.
Please take this discussion to the networking development list,
netdev@vger.kernel.org. It is an interesting issue, but let's discuss
it in the right place. :-)
Prequeue has many advantages, in that processes are properly charged
for TCP processing overhead, and copying to userspace happens directly
in the TCP input path.
This paces TCP senders, in that ACKs do not come back faster than the
kernel can get the process on the cpu to drain the recvmsg() queue.
ACKs sent immediately (without prequeue) give the sender the illusion
that the system can handle a higher data rate than is actually
feasible.
Unfortunately, if there are bugs or bad heuristics in the process
scheduler, this can impact TCP performance quite a bit.
Also, applications using small messages and which are sensitive to
latency can also be harmed by prequeue, that's why we have the
"tcp_low_latency" sysctl. It actually has a slight bug, in that one
of the checks (where you placed the "if (0") was missing, which is
fixed by the patch below:
[TCP]: Fix sysctl_tcp_low_latency
When enabled, this should disable UCOPY prequeue'ing altogether,
but it does not due to a missing test.
Signed-off-by: David S. Miller <davem@davemloft.net>
--- 1/net/ipv4/tcp.c.~1~ 2005-06-09 12:29:41.000000000 -0700
+++ 2/net/ipv4/tcp.c 2005-06-09 16:39:46.000000000 -0700
@@ -1345,7 +1345,7 @@
cleanup_rbuf(sk, copied);
- if (tp->ucopy.task == user_recv) {
+ if (!sysctl_tcp_low_latency && tp->ucopy.task == user_recv) {
/* Install new reader */
if (!user_recv && !(flags & (MSG_TRUNC | MSG_PEEK))) {
user_recv = current;
^ permalink raw reply
* [-mm PATCH][0/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 21:29 UTC (permalink / raw)
To: David S. Miller
Cc: Hideaki YOSHIFUJI, Alexey Kuznetsov, James Morris, Ross Biro,
netdev, linux-kernel
David,
As promised, my previous net/ipv4/raw.c signed/unsigned cleanup patch in
little bits with explanations.
This series of patches cleans up signed versus unsigned variable use in
net/ipv4/raw.c .
The patches are created incrementally on top of each other but will
probably each apply (with a little fuzz) on their own out of order.
Please keep me on CC if you reply since I'm not subscribed to both lists
the patches are send to (only to lkml).
--
Jesper Juhl
^ permalink raw reply
* Re: [-mm PATCH][4/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: David S. Miller @ 2005-06-15 21:29 UTC (permalink / raw)
To: juhl-lkml; +Cc: yoshfuji, kuznet, jmorris, ross.biro, netdev, linux-kernel
In-Reply-To: <Pine.LNX.4.62.0506152316060.3842@dragon.hyggekrogen.localhost>
From: Jesper Juhl <juhl-lkml@dif.dk>
Date: Wed, 15 Jun 2005 23:32:22 +0200 (CEST)
> - if (length >= sizeof(*iph) && iph->ihl * 4 <= length) {
> + if (length >= sizeof(*iph) && (size_t)(iph->ihl * 4) <= length) {
Would changing the "4" into "4U" kill this warning just the same?
I think I'd prefer that.
^ permalink raw reply
* [-mm PATCH][1/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 21:30 UTC (permalink / raw)
To: David S. Miller
Cc: Hideaki YOSHIFUJI, Alexey Kuznetsov, James Morris, Ross Biro,
netdev, linux-kernel
This patch silences these two gcc -W warnings in net/ipv4/raw.c :
net/ipv4/raw.c:517: warning: signed and unsigned type in conditional expression
net/ipv4/raw.c:613: warning: signed and unsigned type in conditional expression
It doesn't change the behaviour of the code, simply writes the conditional
expression with plain 'if()' syntax instead of '? :' , but since this
breaks it into sepperate statements gcc no longer complains about having
both a signed and unsigned value in the same conditional expression.
--- linux-2.6.12-rc6-mm1-orig/net/ipv4/raw.c 2005-06-12 15:58:58.000000000 +0200
+++ linux-2.6.12-rc6-mm1/net/ipv4/raw.c 2005-06-15 22:22:44.000000000 +0200
@@ -514,7 +514,10 @@ done:
kfree(ipc.opt);
ip_rt_put(rt);
-out: return err < 0 ? err : len;
+out:
+ if (err < 0)
+ return err;
+ return len;
do_confirm:
dst_confirm(&rt->u.dst);
@@ -610,7 +613,10 @@ static int raw_recvmsg(struct kiocb *ioc
copied = skb->len;
done:
skb_free_datagram(sk, skb);
-out: return err ? err : copied;
+out:
+ if (err)
+ return err;
+ return copied;
}
static int raw_init(struct sock *sk)
^ permalink raw reply
* [-mm PATCH][2/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 21:30 UTC (permalink / raw)
To: David S. Miller
Cc: Hideaki YOSHIFUJI, Alexey Kuznetsov, James Morris, Ross Biro,
netdev, linux-kernel
This patch gets rid of the following gcc -W warning in net/ipv4/raw.c :
net/ipv4/raw.c:387: warning: comparison of unsigned expression < 0 is always false
Since 'len' is of type size_t it is unsigned and can thus never be <0, and
since this is obvious from the function declaration just a few lines above
I think it's ok to remove the pointless check for len<0.
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---
net/ipv4/raw.c | 2 +-
1 files changed, 1 insertion(+), 1 deletion(-)
--- linux-2.6.12-rc6-mm1/net/ipv4/raw.c.with_patch-1 2005-06-15 22:39:17.000000000 +0200
+++ linux-2.6.12-rc6-mm1/net/ipv4/raw.c 2005-06-15 22:39:36.000000000 +0200
@@ -384,7 +384,7 @@ static int raw_sendmsg(struct kiocb *ioc
int err;
err = -EMSGSIZE;
- if (len < 0 || len > 0xFFFF)
+ if (len > 0xFFFF)
goto out;
/*
^ permalink raw reply
* [-mm PATCH][3/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 21:31 UTC (permalink / raw)
To: David S. Miller
Cc: Hideaki YOSHIFUJI, Alexey Kuznetsov, James Morris, Ross Biro,
netdev, linux-kernel
This patch changes the type of the local variable 'i' in
raw_probe_proto_opt() from 'int' to 'unsigned int'. The only use of 'i' in
this function is as a counter in a for() loop and subsequent index into
the msg->msg_iov[] array.
Since 'i' is compared in a loop to the unsigned variable msg->msg_iovlen
gcc -W generates this warning :
net/ipv4/raw.c:340: warning: comparison between signed and unsigned
Changing 'i' to unsigned silences this warning and is safe since the array
index can never be negative anyway, so unsigned int is the logical type to
use for 'i' and also enables a larger msg_iov[] array (but I don't know if
that will ever matter).
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---
net/ipv4/raw.c | 2 +-
1 files changed, 1 insertion(+), 1 deletion(-)
--- linux-2.6.12-rc6-mm1/net/ipv4/raw.c.with_patch-2 2005-06-15 23:04:40.000000000 +0200
+++ linux-2.6.12-rc6-mm1/net/ipv4/raw.c 2005-06-15 23:09:42.000000000 +0200
@@ -332,7 +332,7 @@ static void raw_probe_proto_opt(struct f
u8 __user *type = NULL;
u8 __user *code = NULL;
int probed = 0;
- int i;
+ unsigned int i;
if (!msg->msg_iov)
return;
^ permalink raw reply
* [-mm PATCH][4/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 21:32 UTC (permalink / raw)
To: David S. Miller
Cc: Hideaki YOSHIFUJI, Alexey Kuznetsov, James Morris, Ross Biro,
netdev, linux-kernel
This patch changes the type of the third parameter 'length' of the
raw_send_hdrinc() function from 'int' to 'size_t'.
This makes sense since this function is only ever called from one
location, and the value passed as the third parameter in that location is
itself of type size_t, so this makes the recieving functions parameter
type match. Also, inside raw_send_hdrinc() the 'length' variable is
used in comparisons with unsigned values and passed as parameter to
functions expecting unsigned values (it's used in a single comparison with
a signed value, but that one can never actually be negative so the patch
also casts that one to size_t to stop gcc worrying, and it is passed in a
single instance to memcpy_fromiovecend() which expects a signed int, but
as far as I can see that's not a problem since the value of 'length'
shouldn't ever exceed the value of a signed int).
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---
net/ipv4/raw.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
--- linux-2.6.12-rc6-mm1/net/ipv4/raw.c.with_patch-3 2005-06-15 23:17:23.000000000 +0200
+++ linux-2.6.12-rc6-mm1/net/ipv4/raw.c 2005-06-15 23:26:48.000000000 +0200
@@ -259,7 +259,7 @@ int raw_rcv(struct sock *sk, struct sk_b
return 0;
}
-static int raw_send_hdrinc(struct sock *sk, void *from, int length,
+static int raw_send_hdrinc(struct sock *sk, void *from, size_t length,
struct rtable *rt,
unsigned int flags)
{
@@ -298,7 +298,7 @@ static int raw_send_hdrinc(struct sock *
goto error_fault;
/* We don't modify invalid header */
- if (length >= sizeof(*iph) && iph->ihl * 4 <= length) {
+ if (length >= sizeof(*iph) && (size_t)(iph->ihl * 4) <= length) {
if (!iph->saddr)
iph->saddr = rt->rt_src;
iph->check = 0;
^ permalink raw reply
* Re: [-mm PATCH][4/4] net: signed vs unsigned cleanup in net/ipv4/raw.c
From: Jesper Juhl @ 2005-06-15 21:40 UTC (permalink / raw)
To: David S. Miller
Cc: juhl-lkml, yoshfuji, kuznet, jmorris, ross.biro, netdev,
linux-kernel
In-Reply-To: <20050615.142953.59469324.davem@davemloft.net>
On Wed, 15 Jun 2005, David S. Miller wrote:
> From: Jesper Juhl <juhl-lkml@dif.dk>
> Date: Wed, 15 Jun 2005 23:32:22 +0200 (CEST)
>
> > - if (length >= sizeof(*iph) && iph->ihl * 4 <= length) {
> > + if (length >= sizeof(*iph) && (size_t)(iph->ihl * 4) <= length) {
>
> Would changing the "4" into "4U" kill this warning just the same?
>
It would.
> I think I'd prefer that.
>
No problem. Here's a replacement patch nr. 4 :
Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
---
net/ipv4/raw.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
--- linux-2.6.12-rc6-mm1/net/ipv4/raw.c.with_patch-3 2005-06-15 23:17:23.000000000 +0200
+++ linux-2.6.12-rc6-mm1/net/ipv4/raw.c 2005-06-15 23:37:11.000000000 +0200
@@ -259,7 +259,7 @@ int raw_rcv(struct sock *sk, struct sk_b
return 0;
}
-static int raw_send_hdrinc(struct sock *sk, void *from, int length,
+static int raw_send_hdrinc(struct sock *sk, void *from, size_t length,
struct rtable *rt,
unsigned int flags)
{
@@ -298,7 +298,7 @@ static int raw_send_hdrinc(struct sock *
goto error_fault;
/* We don't modify invalid header */
- if (length >= sizeof(*iph) && iph->ihl * 4 <= length) {
+ if (length >= sizeof(*iph) && iph->ihl * 4U <= length) {
if (!iph->saddr)
iph->saddr = rt->rt_src;
iph->check = 0;
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox