* Re: Squid hang up on 2.6.34
From: Felipe W Damasio @ 2010-07-09 16:03 UTC (permalink / raw)
To: Eric Dumazet; +Cc: linux-kernel, netdev
In-Reply-To: <AANLkTiliBClahTfb41M5BDi1etg5pgkqEz_VfGrn_mK4@mail.gmail.com>
Hi again,
2010/7/9 Felipe W Damasio <felipewd@gmail.com>:
> I'm trying..the thing is the freeze occured on the machine that sits
> on a 200Mbps ISP in bridge-mode. Since the machine frooze, and the
> whole ISP went down for a few minutes, I'm not allowed to run any
> tests on it.
The only thing I could track down is going through squid's cache.log,
I found these 2 entries a second before the general protection fault:
2010/07/08 14:51:10| httpReadReply: Excess data from "POST
http://bps.uol.com.br/send.html?ro=2VxogIeFwqQdX.ymjRSChUT67HabcLKfYsPrWlpnBtukZ5v8MANz10GJ4i9O3ED-7xl4ng7XW7RvS8AZ.9OP7WbPHhNq6tyh.0eDB5jl2W56wOu.El0KGg8i-bezIAunlQmJ9tFLERUth9-skZOlSnIUeKQeMqaG18kG8z9.tmkxvWMQtTq.fpUiv3mg5.oqN9ZtNuWqtu61GQGOCCQBKjcwTRMlkBCUoJzrOxMgIENaCwuoqHrK29WcpruyeYyDzv3Y2WFh92H-akWXJAFaPyiP-ZIILxBsSZCSmhY3wC-6lS4t.J6z4ek.J6u71vC8nEsYEhLPQBwHVICEpdqpBsW50pa2ooD32sTtUswlcUOU4iEnz8nX1ZRJLF.jOKH7ZPzCIHkAFF1ZAP87xjztOGTncc0X.7d5lwkdITonWzz1El7KLHmz8hB5sluq0Dus-RLbsCNFd0K4URoZLx6bKrypT.xcxL0ampRb.j.8Cais-IdyQDH43n3Z5TVoq5qjNVgPVIY4zA7omN8Wm5hoYIUUVzLUFhFV8hWc4PtPc7hjJK1audQf7jLB4mK5FaFR6VI9OxNTASehc0iZ8Nhee2YbAUxYLPbz.A3qb5iymjZ@&nout=1"
2010/07/08 14:51:10| clientTryParseRequest: FD 6088
(187.16.240.122:2035) Invalid Request
I suppose the last "Invalid request" triggered the bug. But like I
said, I don't know what I can do to help and fix this.
Cheers,
Felipe Damasio
^ permalink raw reply
* Re: Bug handling devices with weird names
From: Eric Dumazet @ 2010-07-09 16:14 UTC (permalink / raw)
To: Martín Ferrari; +Cc: Stephen Hemminger, netdev, Mathieu Lacage
In-Reply-To: <AANLkTinGqQm_HaQABTwQes2D60D3smU2WrD5qydBvoOb@mail.gmail.com>
Le vendredi 09 juillet 2010 à 17:41 +0200, Martín Ferrari a écrit :
> Hi,
>
> On Fri, Jul 9, 2010 at 12:19, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > Update user land tools ?
> >
> > No problem here :
> >
> > # ip link add name foo: type dummy
> > # ip link list foo:
> > 14: foo:: <BROADCAST,NOARP> mtu 1500 qdisc noop state DOWN
> > link/ether e6:48:a9:57:d4:1f brd ff:ff:ff:ff:ff:ff
> > # ip link del foo:
> > # ip -V
> > ip utility, iproute2-ss100519
>
> I am using the exact same version (from Debian), and I can also
> reproduce it with fedora's iproute2-ss080725.
> The kernels are 2.6.35-rc4 and 2.6.27, respectively.. Maybe your
> version of iprout is patched as to not use ioctl?
>
>
>
Well
I use the git version of iproute2, this includes following patch.
Nothing very exciting, but this avoids this ioctl() as you guessed ;)
You cannot ask old binaries to handle foo: devices very well, since
this special char (:) had special meaning in old days.
commit 62a5e0668e2920b7f09896abd884753255712a46
Author: Eric Dumazet <dada1@cosmosbay.com>
Date: Fri Oct 23 06:25:53 2009 +0200
ip: Support IFLA_TXQLEN in ip link show command
We currently use an expensive ioctl() to get device txqueuelen, while
rtnetlink gave it to us for free. This patch speeds up ip link operation
when many devices are registered.
diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 267ecb3..cadc1a3 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -131,26 +131,31 @@ static void print_operstate(FILE *f, __u8 state)
fprintf(f, "state %s ", oper_states[state]);
}
-static void print_queuelen(FILE *f, const char *name)
+static void print_queuelen(FILE *f, struct rtattr *tb[IFLA_MAX + 1])
{
- struct ifreq ifr;
- int s;
-
- s = socket(AF_INET, SOCK_STREAM, 0);
- if (s < 0)
- return;
-
- memset(&ifr, 0, sizeof(ifr));
- strcpy(ifr.ifr_name, name);
- if (ioctl(s, SIOCGIFTXQLEN, &ifr) < 0) {
- fprintf(f, "ioctl(SIOCGIFXQLEN) failed: %s\n", strerror(errno));
+ int qlen;
+
+ if (tb[IFLA_TXQLEN])
+ qlen = *(int *)RTA_DATA(tb[IFLA_TXQLEN]);
+ else {
+ struct ifreq ifr;
+ int s = socket(AF_INET, SOCK_STREAM, 0);
+
+ if (s < 0)
+ return;
+
+ memset(&ifr, 0, sizeof(ifr));
+ strcpy(ifr.ifr_name, (char *)RTA_DATA(tb[IFLA_IFNAME]));
+ if (ioctl(s, SIOCGIFTXQLEN, &ifr) < 0) {
+ fprintf(f, "ioctl(SIOCGIFXQLEN) failed: %s\n", strerror(errno));
+ close(s);
+ return;
+ }
close(s);
- return;
+ qlen = ifr.ifr_qlen;
}
- close(s);
-
- if (ifr.ifr_qlen)
- fprintf(f, "qlen %d", ifr.ifr_qlen);
+ if (qlen)
+ fprintf(f, "qlen %d", qlen);
}
static void print_linktype(FILE *fp, struct rtattr *tb)
@@ -253,7 +258,7 @@ int print_linkinfo(const struct sockaddr_nl *who,
print_operstate(fp, *(__u8 *)RTA_DATA(tb[IFLA_OPERSTATE]));
if (filter.showqueue)
- print_queuelen(fp, (char*)RTA_DATA(tb[IFLA_IFNAME]));
+ print_queuelen(fp, tb);
if (!filter.family || filter.family == AF_PACKET) {
SPRINT_BUF(b1);
^ permalink raw reply related
* Re: [PATCH] netfilter: add CHECKSUM target
From: Jan Engelhardt @ 2010-07-09 16:26 UTC (permalink / raw)
To: Patrick McHardy
Cc: Michael S. Tsirkin, David S. Miller, Alexey Kuznetsov,
Pekka Savola (ipv6), James Morris, Hideaki YOSHIFUJI,
linux-kernel, netfilter-devel, netfilter, coreteam, netdev,
herbert.xu, kvm
In-Reply-To: <4C373D90.8070000@trash.net>
On Friday 2010-07-09 17:17, Patrick McHardy wrote:
>
>> This adds a `CHECKSUM' target, which can be used in the iptables mangle
>> table.
>>
>> You can use this target to compute and fill in the checksum in
>> an IP packet that lacks a checksum. This is particularly useful,
>> if you need to work around old applications such as dhcp clients,
>> that do not work well with checksum offloads, but don't want to
>> disable checksum offload in your device.
>
>I'm not sure this is something we want to merge upstream and
>support indefinitely.
We could put it into Xtables-addons. That would also be consistent
with Dave's suggestion.
>Dave suggested this as a temporary
>out-of-tree workaround until the majority of guest dhcp clients
>are fixed. Has anything changed that makes this course of
>action impractical?
^ permalink raw reply
* [PATCH 001/001] QoS and/or fair queueing: Stateless NAT BUG
From: rpartearroyo @ 2010-07-09 16:35 UTC (permalink / raw)
To: Eric Dumazet
Cc: Herbert Xu, Linux Kernel Mailing List, Iratxo Pichel Ortiz,
Noelia Morón, netdev
Hi all,
I have been testing Stateless NAT and found that ICMP packets with length
less than 20 bytes were not correctly NAT'ed. I have found a BUG that
makes taking into account IP header length twice, so ICMP packets smaller
than 20 bytes were being dropped.
Proposed formal patch is below, as suggested by Eric Dumazet, thanks.
It is taken from 2.6.34.1 stable version.
Signed-off-by: Rodrigo Partearroyo González <rpartearroyo@albentia.com>
---
diff -uprN a/net/sched/act_nat.c b/net/sched/act_nat.c
--- a/net/sched/act_nat.c 2010-07-09 18:25:18.000000000 +0200
+++ b/net/sched/act_nat.c 2010-07-09 18:26:16.000000000 +0200
@@ -202,7 +202,7 @@ static int tcf_nat(struct sk_buff *skb,
{
struct icmphdr *icmph;
- if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
+ if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
goto drop;
icmph = (void *)(skb_network_header(skb) + ihl);
@@ -223,7 +223,7 @@ static int tcf_nat(struct sk_buff *skb,
if (skb_cloned(skb) &&
!skb_clone_writable(skb,
- ihl + sizeof(*icmph) +
sizeof(*iph)) &&
+ ihl + sizeof(*icmph) ) &&
pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
goto drop;
---
--
Rodrigo Partearroyo González
Albentia Systems S.A.
http://www.albentia.com
C\Margarita Salas 22
Parque Tecnológico de Leganés
Leganés (28918)
Madrid
Spain
^ permalink raw reply
* Re: [PATCH] netfilter: add CHECKSUM target
From: Patrick McHardy @ 2010-07-09 16:36 UTC (permalink / raw)
To: Jan Engelhardt
Cc: Michael S. Tsirkin, David S. Miller, Alexey Kuznetsov,
Pekka Savola (ipv6), James Morris, Hideaki YOSHIFUJI,
linux-kernel, netfilter-devel, netfilter, coreteam, netdev,
herbert.xu, kvm
In-Reply-To: <alpine.LSU.2.01.1007091824520.1835@obet.zrqbmnf.qr>
Am 09.07.2010 18:26, schrieb Jan Engelhardt:
>
> On Friday 2010-07-09 17:17, Patrick McHardy wrote:
>>
>>> This adds a `CHECKSUM' target, which can be used in the iptables mangle
>>> table.
>>>
>>> You can use this target to compute and fill in the checksum in
>>> an IP packet that lacks a checksum. This is particularly useful,
>>> if you need to work around old applications such as dhcp clients,
>>> that do not work well with checksum offloads, but don't want to
>>> disable checksum offload in your device.
>>
>> I'm not sure this is something we want to merge upstream and
>> support indefinitely.
>
> We could put it into Xtables-addons. That would also be consistent
> with Dave's suggestion.
Sure, that would be fine with me.
^ permalink raw reply
* Re: [PATCH v4 1/9] atm: propagate signal changes via notifier
From: David Miller @ 2010-07-09 16:48 UTC (permalink / raw)
To: chas; +Cc: karl, linux-atm-general, netdev
In-Reply-To: <20100709071610.42473d6c@thirdoffive.cmf.nrl.navy.mil>
From: chas williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
Date: Fri, 9 Jul 2010 07:16:10 -0400
> The preferred style for long (multi-line) comments is:
Which is stupid because it causes one to be able to keep less actual
code on the screen at a time.
^ permalink raw reply
* Re: [PATCH] at1700: fix double free_irq
From: Dan Carpenter @ 2010-07-09 16:58 UTC (permalink / raw)
To: Kulikov Vasiliy
Cc: kernel-janitors, David S. Miller, Jiri Pirko, Joe Perches,
Stephen Hemminger, Eric Dumazet, netdev
In-Reply-To: <1278678686-7215-1-git-send-email-segooon@gmail.com>
On Fri, Jul 09, 2010 at 04:31:26PM +0400, Kulikov Vasiliy wrote:
> free_irq() is called both in net_close() and cleanup_card(). Since it
> is requested in at1700_probe1(), leave free_irq() only in cleanup_card()
> for balance.
>
Are you sure? I would think that we should make the free_irq() in
cleanup_card() conditional instead.
> Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
> ---
> drivers/net/at1700.c | 4 +---
> 1 files changed, 1 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/at1700.c b/drivers/net/at1700.c
> index 93185f5..8987689 100644
> --- a/drivers/net/at1700.c
> +++ b/drivers/net/at1700.c
> @@ -811,10 +811,8 @@ static int net_close(struct net_device *dev)
> /* No statistic counters on the chip to update. */
>
> /* Disable the IRQ on boards of fmv18x where it is feasible. */
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It seems like this comment should be updated?
> - if (lp->jumpered) {
> + if (lp->jumpered)
> outb(0x00, ioaddr + IOCONFIG1);
> - free_irq(dev->irq, dev);
> - }
regards,
dan carpenter
^ permalink raw reply
* [PATCH] tproxy: nf_tproxy_assign_sock() can handle tw sockets
From: Eric Dumazet @ 2010-07-09 17:13 UTC (permalink / raw)
To: Felipe W Damasio, David Miller, Patrick McHardy; +Cc: linux-kernel, netdev
In-Reply-To: <AANLkTiliBClahTfb41M5BDi1etg5pgkqEz_VfGrn_mK4@mail.gmail.com>
Le vendredi 09 juillet 2010 à 12:03 -0300, Felipe W Damasio a écrit :
> Hi,
>
> 2010/7/8 Eric Dumazet <eric.dumazet@gmail.com>:
> > Please try to reproduce a new report.
> >
> > It looks like a memory corruption, and it would be good to see if a
> > common pattern is occurring.
>
> I'm trying..the thing is the freeze occured on the machine that sits
> on a 200Mbps ISP in bridge-mode. Since the machine frooze, and the
> whole ISP went down for a few minutes, I'm not allowed to run any
> tests on it.
>
> I've setup the same scenario on a lab, but since last night been
> unable to reproduce the bug. Maybe there's a clue on the this crash
> below that can help me write some program to trigger the problem?
>
Reviewing tproxy stuff I spotted a problem in nf_tproxy_assign_sock()
but I could not see how it could explain your crash.
We can read uninitialized memory and trigger a fault in
nf_tproxy_assign_sock(), not later in tcp_recvmsg()...
David, Patrick, what do you think ?
Thanks
[PATCH] tproxy: nf_tproxy_assign_sock() can handle tw sockets
transparent field of a socket is either inet_twsk(sk)->tw_transparent
for timewait sockets, or inet_sk(sk)->transparent for other sockets
(TCP/UDP).
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/net/netfilter/nf_tproxy_core.c b/net/netfilter/nf_tproxy_core.c
index 5490fc3..daab8c4 100644
--- a/net/netfilter/nf_tproxy_core.c
+++ b/net/netfilter/nf_tproxy_core.c
@@ -70,7 +70,11 @@ nf_tproxy_destructor(struct sk_buff *skb)
int
nf_tproxy_assign_sock(struct sk_buff *skb, struct sock *sk)
{
- if (inet_sk(sk)->transparent) {
+ bool transparent = (sk->sk_state == TCP_TIME_WAIT) ?
+ inet_twsk(sk)->tw_transparent :
+ inet_sk(sk)->transparent;
+
+ if (transparent) {
skb_orphan(skb);
skb->sk = sk;
skb->destructor = nf_tproxy_destructor;
^ permalink raw reply related
* Re: [PATCH 001/001] QoS and/or fair queueing: Stateless NAT BUG
From: Rodrigo Partearroyo González @ 2010-07-09 17:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Herbert Xu, Linux Kernel Mailing List, Iratxo Pichel Ortiz,
Noelia Morón, netdev
In-Reply-To: <25524.83.175.223.254.1278693359.squirrel@mail.albentia.com>
Seems like the mailer corrupted the patch. Sorry, I resend it.
Thanks Eric.
On Viernes, 9 de Julio de 2010 18:35:59 rpartearroyo@albentia.com escribió:
> Hi all,
> I have been testing Stateless NAT and found that ICMP packets with length
> less than 20 bytes were not correctly NAT'ed. I have found a BUG that
> makes taking into account IP header length twice, so ICMP packets smaller
> than 20 bytes were being dropped.
>
> Proposed formal patch is below, as suggested by Eric Dumazet, thanks.
> It is taken from 2.6.34.1 stable version.
>
Signed-off-by: Rodrigo Partearroyo González <rpartearroyo@albentia.com>
---
diff -uprN a/net/sched/act_nat.c b/net/sched/act_nat.c
--- a/net/sched/act_nat.c 2010-07-09 18:25:18.000000000 +0200
+++ b/net/sched/act_nat.c 2010-07-09 18:26:16.000000000 +0200
@@ -202,7 +202,7 @@ static int tcf_nat(struct sk_buff *skb,
{
struct icmphdr *icmph;
- if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
+ if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
goto drop;
icmph = (void *)(skb_network_header(skb) + ihl);
@@ -223,7 +223,7 @@ static int tcf_nat(struct sk_buff *skb,
if (skb_cloned(skb) &&
!skb_clone_writable(skb,
- ihl + sizeof(*icmph) + sizeof(*iph)) &&
+ ihl + sizeof(*icmph) ) &&
pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
goto drop;
---
--
Rodrigo Partearroyo González
R&D Engineer
Albentia Systems S.A.
http://www.albentia.com
+34 914400213
C\Margarita Salas 22
Parque Tecnológico de Leganés
Leganés (28918)
Madrid
Spain
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: Marcel Holtmann @ 2010-07-09 17:33 UTC (permalink / raw)
To: Alan Ott
Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
Eric Dumazet, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <4C372CEE.3070109-yzvJWuRpmD1zbRFIqnYvSA@public.gmane.org>
Hi Alan,
> >>>>> I looked at this and I am bit worried that this should not be done in
> >>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
> >>>>> driver. It should not handle all these details. Can we make this a bit
> >>>>> easier for the transport drivers to support such features?
> >>>>>
> >>>>>
> >>>> I put these changes (most notably the addition of hidp_get_raw_report())
> >>>> in hidp because that's where the parallel function
> >>>> hidp_output_raw_report() was already located. I figured the input should
> >>>> go with the output. That said, if there's a better place for both of
> >>>> them (input and output) to go, let me know where you think it should be,
> >>>> and I'll get them moved into the proper spot.
> >>>>
> >>>> I'm not sure what you mean about HIDP being a pure transport driver.
> >>>>
> >>>>
> >>> what is usb-hid.ko doing here? I would expect a bunch of code
> >>> duplication with minor difference between USB and Bluetooth.
> >>>
> >> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
> >> usbhid_output_raw_report()
> >> - calls usb_control_msg() with Get_Report
> >> usbhid_get_raw_report()
> >> - calls usb_control_msg() with Set_Report
> >> OR
> >> - calls usb_interrupt_msg() on the Ouput pipe.
> >>
> >> This is of course easier than bluetooth because usb_control_msg() is
> >> synchronous, even when requesting reports, mostly because of the nature
> >> of USB, where the request and response are part of the same transfer.
> >>
> >> For Bluetooth, it's a bit more complicated since the kernel treats it
> >> more like a networking interface (and indeed it is). My understanding is
> >> that to make a synchronous transfer in bluetooth, one must:
> >> - send the request packet
> >> - block (wait_event_*())
> >> - when the response is received in the input handler, wake_up_*().
> >>
> >> There's not really any code duplication, mostly because initiating
> >> synchronous USB transfers (input and output) is easy (because of the
> >> usb_*_msg() functions), while making synchronous Bluetooth transfers
> >> must be done manually. If there's a nice, convenient, synchronous
> >> function in Bluetooth similar to usb_control_msg() that I've missed,
> >> then let me know, as it would simplify this whole thing.
> >>
> > there is not and I don't think we ever get one. My question here was
> > more in the direction why HID core is doing these synchronously in the
> > first place. Especially since USB can do everything async as well.
>
> I'm open to suggestions. The way I see it is from a user space
> perspective. With Get_Feature being on an ioctl(), I don't see any clean
> way to do it other than synchronously. Other operating systems (I can
> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
> same way (synchronously) from user space.
>
> You seem to be proposing an asynchronous interface. What would that look
> like from user space?
not necessarily from user space, but at least from HID core to HIDP and
usb-hid transports. At least that is what I would expect, Jiri?
Regards
Marcel
^ permalink raw reply
* Re: [PATCH 001/001] QoS and/or fair queueing: Stateless NAT BUG
From: Rodrigo Partearroyo González @ 2010-07-09 17:37 UTC (permalink / raw)
To: Eric Dumazet
Cc: Herbert Xu, Linux Kernel Mailing List, Iratxo Pichel Ortiz,
Noelia Morón, netdev
Hi all,
I have been testing Stateless NAT and found that ICMP packets with length
less than 20 bytes were not correctly NAT'ed. I have found a BUG that
makes taking into account IP header length twice, so ICMP packets smaller
than 20 bytes were being dropped.
Proposed formal patch is below, as suggested by Eric Dumazet, thanks.
It is taken from 2.6.34.1 stable version.
Signed-off-by: Rodrigo Partearroyo González <rpartearroyo@albentia.com>
---
diff -uprN a/net/sched/act_nat.c b/net/sched/act_nat.c
--- a/net/sched/act_nat.c 2010-07-09 18:25:18.000000000 +0200
+++ b/net/sched/act_nat.c 2010-07-09 18:26:16.000000000 +0200
@@ -202,7 +202,7 @@ static int tcf_nat(struct sk_buff *skb,
{
struct icmphdr *icmph;
- if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
+ if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
goto drop;
icmph = (void *)(skb_network_header(skb) + ihl);
@@ -223,7 +223,7 @@ static int tcf_nat(struct sk_buff *skb,
if (skb_cloned(skb) &&
!skb_clone_writable(skb,
- ihl + sizeof(*icmph) + sizeof(*iph)) &&
+ ihl + sizeof(*icmph)) &&
pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
goto drop;
---
Rodrigo Partearroyo González
R&D Engineer
Albentia Systems S.A.
http://www.albentia.com
+34 914400213
C\Margarita Salas 22
Parque Tecnológico de Leganés
Leganés (28918)
Madrid
Spain
^ permalink raw reply
* Re: [PATCH v4 1/9] atm: propagate signal changes via notifier
From: chas williams - CONTRACTOR @ 2010-07-09 17:44 UTC (permalink / raw)
To: David Miller; +Cc: karl, linux-atm-general, netdev
In-Reply-To: <20100709.094856.193707207.davem@davemloft.net>
On Fri, 09 Jul 2010 09:48:56 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> Which is stupid because it causes one to be able to keep less actual
> code on the screen at a time.
stupid or not it is preferred. get it changed.
^ permalink raw reply
* Re: [PATCH] at1700: fix double free_irq
From: Kulikov Vasiliy @ 2010-07-09 17:48 UTC (permalink / raw)
To: Dan Carpenter
Cc: kernel-janitors, David S. Miller, Jiri Pirko, Joe Perches,
Stephen Hemminger, Eric Dumazet, netdev
In-Reply-To: <20100709165817.GN19184@bicker>
On Fri, Jul 09, 2010 at 18:58 +0200, Dan Carpenter wrote:
> On Fri, Jul 09, 2010 at 04:31:26PM +0400, Kulikov Vasiliy wrote:
> > free_irq() is called both in net_close() and cleanup_card(). Since it
> > is requested in at1700_probe1(), leave free_irq() only in cleanup_card()
> > for balance.
> >
>
> Are you sure? I would think that we should make the free_irq() in
> cleanup_card() conditional instead.
See balanced functions: net_open() & net_close(), at1700_probe1() &
cleanup_card(). request_irq() is in probe, so it must not be
freed on 'ifconfig down'. E.g.
modprobe at1700 <== request_irq()
ifconfig eth0 up
ifconfig eth0 down <== first free_irq()
ifconfig eth0 up <== no request_irq() here!
ifconfig eth0 down <== second free_irq()
rmmod at1700 <== third free_irq()
>
> > Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
> > ---
> > drivers/net/at1700.c | 4 +---
> > 1 files changed, 1 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/at1700.c b/drivers/net/at1700.c
> > index 93185f5..8987689 100644
> > --- a/drivers/net/at1700.c
> > +++ b/drivers/net/at1700.c
> > @@ -811,10 +811,8 @@ static int net_close(struct net_device *dev)
> > /* No statistic counters on the chip to update. */
> >
> > /* Disable the IRQ on boards of fmv18x where it is feasible. */
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> It seems like this comment should be updated?
Maybe yes, but I don't know what these damn IO requests mean.
Sure, it's better to request IRQ in xxx_open(), but as it is already
done in probe() I leave it here.
If it is a bug then I do nothing with it, but if it is not then I'll
create a bug.
>
> > - if (lp->jumpered) {
> > + if (lp->jumpered)
> > outb(0x00, ioaddr + IOCONFIG1);
> > - free_irq(dev->irq, dev);
> > - }
>
> regards,
> dan carpenter
^ permalink raw reply
* Re: [PATCH]: rfs: record flow in TCP receiving and sending pathes
From: David Miller @ 2010-07-09 17:51 UTC (permalink / raw)
To: xiaosuo; +Cc: therbert, netdev
In-Reply-To: <1278660498-26587-1-git-send-email-xiaosuo@gmail.com>
From: Changli Gao <xiaosuo@gmail.com>
Date: Fri, 9 Jul 2010 15:28:18 +0800
> rfs: record flow in TCP receiving and sending pathes
>
> call sock_rps_record_flow() in function tcp_splice_read(), tcp_sendpage() and
> tcp_sendmsg().
>
> Signed-off-by: Changli Gao <xiaosuo@gmail.com>
I don't think it's wise to pepper these calls all over the place if it
is not necessary.
The only reason we vector straight to the TCP implementations of these
I/O routines is to avoid the port autobinding made by the inet_*()
functions.
But now that avoids also the RPS calls.
So it makes sense to just add a boolean state bit flag of some sort
to "struct proto" which says to avoid the autobind calls, then
make TCP vector through the inet_*() functions just like the other
inet protocols do.
Then these extra send_rps_record_flow() annotations will not be
necessary.
^ permalink raw reply
* Re: [PATCH 001/001] QoS and/or fair queueing: Stateless NAT BUG
From: David Miller @ 2010-07-09 17:52 UTC (permalink / raw)
To: rpartearroyo; +Cc: eric.dumazet, herbert, linux-kernel, ipichel, nmoron, netdev
In-Reply-To: <201007091937.17349.rpartearroyo@albentia.com>
From: Rodrigo Partearroyo González <rpartearroyo@albentia.com>
Date: Fri, 9 Jul 2010 19:37:16 +0200
> Hi all,
> I have been testing Stateless NAT and found that ICMP packets with length
> less than 20 bytes were not correctly NAT'ed. I have found a BUG that
> makes taking into account IP header length twice, so ICMP packets smaller
> than 20 bytes were being dropped.
>
> Proposed formal patch is below, as suggested by Eric Dumazet, thanks.
> It is taken from 2.6.34.1 stable version.
>
> Signed-off-by: Rodrigo Partearroyo González <rpartearroyo@albentia.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] tproxy: nf_tproxy_assign_sock() can handle tw sockets
From: David Miller @ 2010-07-09 17:53 UTC (permalink / raw)
To: eric.dumazet; +Cc: felipewd, kaber, linux-kernel, netdev
In-Reply-To: <1278695580.2696.55.camel@edumazet-laptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 09 Jul 2010 19:13:00 +0200
> Reviewing tproxy stuff I spotted a problem in nf_tproxy_assign_sock()
> but I could not see how it could explain your crash.
>
> We can read uninitialized memory and trigger a fault in
> nf_tproxy_assign_sock(), not later in tcp_recvmsg()...
>
> David, Patrick, what do you think ?
>
> Thanks
>
> [PATCH] tproxy: nf_tproxy_assign_sock() can handle tw sockets
>
> transparent field of a socket is either inet_twsk(sk)->tw_transparent
> for timewait sockets, or inet_sk(sk)->transparent for other sockets
> (TCP/UDP).
>
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Looks fine to me:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH 001/001] QoS and/or fair queueing: Stateless NAT BUG
From: Eric Dumazet @ 2010-07-09 17:54 UTC (permalink / raw)
To: Rodrigo Partearroyo González
Cc: Herbert Xu, Linux Kernel Mailing List, Iratxo Pichel Ortiz,
Noelia Morón, netdev
In-Reply-To: <201007091937.17349.rpartearroyo@albentia.com>
Le vendredi 09 juillet 2010 à 19:37 +0200, Rodrigo Partearroyo González
a écrit :
> Hi all,
> I have been testing Stateless NAT and found that ICMP packets with length
> less than 20 bytes were not correctly NAT'ed. I have found a BUG that
> makes taking into account IP header length twice, so ICMP packets smaller
> than 20 bytes were being dropped.
>
> Proposed formal patch is below, as suggested by Eric Dumazet, thanks.
> It is taken from 2.6.34.1 stable version.
>
> Signed-off-by: Rodrigo Partearroyo González <rpartearroyo@albentia.com>
Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
> diff -uprN a/net/sched/act_nat.c b/net/sched/act_nat.c
> --- a/net/sched/act_nat.c 2010-07-09 18:25:18.000000000 +0200
> +++ b/net/sched/act_nat.c 2010-07-09 18:26:16.000000000 +0200
> @@ -202,7 +202,7 @@ static int tcf_nat(struct sk_buff *skb,
> {
> struct icmphdr *icmph;
>
> - if (!pskb_may_pull(skb, ihl + sizeof(*icmph) + sizeof(*iph)))
> + if (!pskb_may_pull(skb, ihl + sizeof(*icmph)))
> goto drop;
>
> icmph = (void *)(skb_network_header(skb) + ihl);
> @@ -223,7 +223,7 @@ static int tcf_nat(struct sk_buff *skb,
>
> if (skb_cloned(skb) &&
> !skb_clone_writable(skb,
> - ihl + sizeof(*icmph) + sizeof(*iph)) &&
> + ihl + sizeof(*icmph)) &&
> pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
> goto drop;
>
^ permalink raw reply
* [PATCH v3] ll_temac: fix DMA and memory leaks
From: Kulikov Vasiliy @ 2010-07-09 17:55 UTC (permalink / raw)
To: kernel-janitors
Cc: Denis Kirjanov, David S. Miller, John Linn, Grant Likely,
Jiri Pirko, netdev, devicetree-discuss
temac_dma_bd_init() doesn't free DMA and memory on error. Also
temac_stop() must free them.
Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
---
drivers/net/ll_temac_main.c | 38 ++++++++++++++++++++++++++++++++++++++
1 files changed, 38 insertions(+), 0 deletions(-)
diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
index 5bca20b..9090e79 100644
--- a/drivers/net/ll_temac_main.c
+++ b/drivers/net/ll_temac_main.c
@@ -193,6 +193,41 @@ static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,
#endif
/**
+ * * temac_dma_bd_release - Release buffer descriptor rings
+ */
+static void temac_dma_bd_release(struct net_device *ndev)
+{
+ struct temac_local *lp = netdev_priv(ndev);
+ int i;
+
+ if (lp->rx_skb) {
+ for (i = 0; i < RX_BD_NUM; i++) {
+ if (!lp->rx_skb[i])
+ break;
+ dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
+ XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
+ dev_kfree_skb(lp->rx_skb[i]);
+ }
+ kfree(lp->rx_skb);
+ lp->rx_skb = NULL;
+ }
+
+ if (lp->rx_bd_v) {
+ dma_free_coherent(ndev->dev.parent,
+ sizeof(*lp->rx_bd_v) * RX_BD_NUM,
+ lp->rx_bd_v, lp->rx_bd_p);
+ lp->rx_bd_v = NULL;
+ }
+
+ if (lp->tx_bd_v) {
+ dma_free_coherent(ndev->dev.parent,
+ sizeof(*lp->tx_bd_v) * TX_BD_NUM,
+ lp->tx_bd_v, lp->tx_bd_p);
+ lp->tx_bd_v = NULL;
+ }
+}
+
+/**
* temac_dma_bd_init - Setup buffer descriptor rings
*/
static int temac_dma_bd_init(struct net_device *ndev)
@@ -275,6 +310,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
return 0;
out:
+ temac_dma_bd_release(ndev);
return -ENOMEM;
}
@@ -858,6 +894,8 @@ static int temac_stop(struct net_device *ndev)
phy_disconnect(lp->phy_dev);
lp->phy_dev = NULL;
+ temac_dma_bd_release(ndev);
+
return 0;
}
--
1.7.0.4
^ permalink raw reply related
* Urgent Response Required.(OSA AWARD)
From: Mrs. Maria Ortega @ 2010-07-09 17:47 UTC (permalink / raw)
Euro Millones s.l
Oversea Subscriber Agent
Madrid, Spain.
Ref : EML/2010/799
Bat : 4/001/2301ESP
You have won the Oversea Subscriber Agency(OSA) Bonanza Award of 2nd July 2010 of the Euro Millones Int. Your winning email attached to ticket 12-13-36-41-46, 5-8. won in the 2nd category. You are therefore approved to receive the sum of 1.417.999,54 Euro.
Check result here: http://www.onlae.es/euroMillones/comprobar.aspx
Contact your claims officer:
Mr. Frank Dumbell
Tel: 0034-672-528-600
Email: osaclearance@aol.co.uk
Congratulations on your success.
Regards,
Mrs. Maria Ortega
Coordinator
^ permalink raw reply
* Re: [PATCH v2 -net-2.6] ll_temac: fix DMA resources leak
From: Kulikov Vasiliy @ 2010-07-09 17:59 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: davem, john.linn, brian.hill, grant.likely, jpirko, netdev
In-Reply-To: <20100709114347.GA3553@albatros>
On Fri, Jul 09, 2010 at 15:43 +0400, Kulikov Vasiliy wrote:
> On Thu, Jul 08, 2010 at 20:24 +0000, Denis Kirjanov wrote:
> > V2: Check pointers before releasing resources.
> >
> > Fix DMA resources leak.
> > Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
> > Signed-off-by: Kulikov Vasiliy <segooon@gmail.com>
> > ---
> > 1 files changed, 32 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/ll_temac_main.c b/drivers/net/ll_temac_main.c
> > index fa303c8..b57d0ff 100644
> > --- a/drivers/net/ll_temac_main.c
> > +++ b/drivers/net/ll_temac_main.c
> > @@ -193,6 +193,35 @@ static int temac_dcr_setup(struct temac_local *lp, struct of_device *op,
> > #endif
> >
> > /**
> > + * * temac_dma_bd_release - Release buffer descriptor rings
> > + */
> > +static void temac_dma_bd_release(struct net_device *ndev)
> > +{
> > + struct temac_local *lp = netdev_priv(ndev);
> > + int i;
> > +
> > + for (i = 0; i < RX_BD_NUM; i++) {
> > + if (!lp->rx_skb[i])
> > + break;
> > + else {
> > + dma_unmap_single(ndev->dev.parent, lp->rx_bd_v[i].phys,
> > + XTE_MAX_JUMBO_FRAME_SIZE, DMA_FROM_DEVICE);
> > + dev_kfree_skb(lp->rx_skb[i]);
> > + }
> > + }
> This cycle is needed only if (lp->rx_skb != NULL).
>
> > + if (lp->rx_bd_v)
> > + dma_free_coherent(ndev->dev.parent,
> > + sizeof(*lp->rx_bd_v) * RX_BD_NUM,
> > + lp->rx_bd_v, lp->rx_bd_p);
> > + if (lp->tx_bd_v)
> > + dma_free_coherent(ndev->dev.parent,
> > + sizeof(*lp->tx_bd_v) * TX_BD_NUM,
> > + lp->tx_bd_v, lp->tx_bd_p);
> After temac_dma_bd_release() lp->rx_bd_v and lp->rx_bd_p are freed but
> are nonzero. If lp->rx_skb allocation fails second time then these DMA's
> would be freed second time.
> lp->tx_bd_v = lp->rx_bd_v = NULL here fixes this.
>
> > + if (lp->rx_skb)
> > + kfree(lp->rx_skb);
> > +}
> > +
> > +/**
> > * temac_dma_bd_init - Setup buffer descriptor rings
> > */
> > static int temac_dma_bd_init(struct net_device *ndev)
> > @@ -275,6 +304,7 @@ static int temac_dma_bd_init(struct net_device *ndev)
> > return 0;
> >
> > out:
> > + temac_dma_bd_release(ndev);
> > return -ENOMEM;
> > }
> >
> > @@ -858,6 +888,8 @@ static int temac_stop(struct net_device *ndev)
> > phy_disconnect(lp->phy_dev);
> > lp->phy_dev = NULL;
> >
> > + temac_dma_bd_release(ndev);
> > +
> > return 0;
> > }
> >
I've fixed it in PATCH v3.
http://marc.info/?l=kernel-janitors&m=127869815002994&w=2
^ permalink raw reply
* Re: [PATCH] tproxy: nf_tproxy_assign_sock() can handle tw sockets
From: Felipe W Damasio @ 2010-07-09 18:16 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Patrick McHardy, linux-kernel, netdev
In-Reply-To: <1278695580.2696.55.camel@edumazet-laptop>
Hi,
2010/7/9 Eric Dumazet <eric.dumazet@gmail.com>:
> Reviewing tproxy stuff I spotted a problem in nf_tproxy_assign_sock()
> but I could not see how it could explain your crash.
>
> We can read uninitialized memory and trigger a fault in
> nf_tproxy_assign_sock(), not later in tcp_recvmsg()...
Well, since I can't reproduce the bug, if you think this patch solves
my problem, I'll tell my bosses that we can put back the production
machine online.
But anyway, if there's test you think I can run, I have same the same
hardware on a lab using the same setup as the production
environment...
Thanks,
Felipe Damasio
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: Alan Ott @ 2010-07-09 18:24 UTC (permalink / raw)
To: Marcel Holtmann
Cc: David S Miller, Jiri Kosina, Michael Poole, Bastien Nocera,
Eric Dumazet, linux-bluetooth-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1278696815.10421.137.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org>
On 07/09/2010 01:33 PM, Marcel Holtmann wrote:
>>>>>>> I looked at this and I am bit worried that this should not be done in
>>>>>>> this detail in the HIDP driver. Essentially HIDP is a pure transport
>>>>>>> driver. It should not handle all these details. Can we make this a bit
>>>>>>> easier for the transport drivers to support such features?
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>> I put these changes (most notably the addition of hidp_get_raw_report())
>>>>>> in hidp because that's where the parallel function
>>>>>> hidp_output_raw_report() was already located. I figured the input should
>>>>>> go with the output. That said, if there's a better place for both of
>>>>>> them (input and output) to go, let me know where you think it should be,
>>>>>> and I'll get them moved into the proper spot.
>>>>>>
>>>>>> I'm not sure what you mean about HIDP being a pure transport driver.
>>>>>>
>>>>>>
>>>>>>
>>>>> what is usb-hid.ko doing here? I would expect a bunch of code
>>>>> duplication with minor difference between USB and Bluetooth.
>>>>>
>>>>>
>>>> usbhid doesn't have a lot of code for hidraw. Two functions are involved:
>>>> usbhid_output_raw_report()
>>>> - calls usb_control_msg() with Get_Report
>>>> usbhid_get_raw_report()
>>>> - calls usb_control_msg() with Set_Report
>>>> OR
>>>> - calls usb_interrupt_msg() on the Ouput pipe.
>>>>
>>>> This is of course easier than bluetooth because usb_control_msg() is
>>>> synchronous, even when requesting reports, mostly because of the nature
>>>> of USB, where the request and response are part of the same transfer.
>>>>
>>>> For Bluetooth, it's a bit more complicated since the kernel treats it
>>>> more like a networking interface (and indeed it is). My understanding is
>>>> that to make a synchronous transfer in bluetooth, one must:
>>>> - send the request packet
>>>> - block (wait_event_*())
>>>> - when the response is received in the input handler, wake_up_*().
>>>>
>>>> There's not really any code duplication, mostly because initiating
>>>> synchronous USB transfers (input and output) is easy (because of the
>>>> usb_*_msg() functions), while making synchronous Bluetooth transfers
>>>> must be done manually. If there's a nice, convenient, synchronous
>>>> function in Bluetooth similar to usb_control_msg() that I've missed,
>>>> then let me know, as it would simplify this whole thing.
>>>>
>>>>
>>> there is not and I don't think we ever get one. My question here was
>>> more in the direction why HID core is doing these synchronously in the
>>> first place. Especially since USB can do everything async as well.
>>>
>> I'm open to suggestions. The way I see it is from a user space
>> perspective. With Get_Feature being on an ioctl(), I don't see any clean
>> way to do it other than synchronously. Other operating systems (I can
>> say for sure Windows, Mac OS X, and FreeBSD) handle Get/Set Feature the
>> same way (synchronously) from user space.
>>
>> You seem to be proposing an asynchronous interface. What would that look
>> like from user space?
>>
> not necessarily from user space, but at least from HID core to HIDP and
> usb-hid transports. At least that is what I would expect, Jiri?
>
> Regards
>
> Marcel
>
Hi Marcel,
So it sounds like you're mostly concerned about the sleeping (blocking),
and where is the _right_ place for it to occur. It seems like it could
either occur in hid/hidraw.c or in hidp/core.c. If it were to occur in
hid/hidraw.c, what then would get passed back and forth between the
bluetooth/hidp and hidraw?
Maybe something like the following:
hidraw:
- get_report() (hypothetical)
- calls a hypothetical hidp_initiate_get_report(), which:
- sends the report request and returns immediately.
- wait for response
hidp:
- whenever a report is returned, it calls back to hidraw,
which wakes up the get_report() thread if
the data matches the report being waited on.
For this to work, we'd need 2 more function pointers in struct hid_device:
1. a way for hidp to call back into hidraw.
2. a pointer for hidp_initiate_get_report().
These of course would be in addition to the ones that USB already uses
(like hid_get_raw_report()), and would cause USB and Bluetooth to use
different APIs to each transport.
Of course, there could be commonality if we used the asynchronous USB
APIs like you suggested, although, I'm not sure I see the benefit of
making the USB part more complicated. The USB part (hid/usb/hid-core.c)
is currently _very_ simple.
It seems like we have two options:
1. Move to asynchronous APIs in USB and Bluetooth. This involves:
a. Move to asynchronous APIs in hid/usbhid/hid-core.c
b. Adding support into hid/hidraw.c to do the waiting.
c. Changing bluetooth/hidp to be asynchronous in nature.
2. Keep using synchronous USB APIs.
a. hid/usbhid/hid-core remains really simple
b. hid/hidraw.c remains really simple
c. bluetooth/hidp has some complexity
I'd argue that the complexity of bluetooth/hidp isn't really that
complex, and further, it's mostly isolated to one (new) function (that's
where the wait_event_*() is).
Further, if we did option #2, some piece of code has to determine
whether to wake up the blocking thread (which would then be in
hid/hidraw.c). This piece of code would be notified for every packet
received from Bluetooth to decide whether it should wake up the sleeping
thread, and would have to have bluetooth-specific code in it (something
like the block which calls wake_up_interruptible() in my patch). It
seems like this code would _have_ to be in hidp.
From a design standpoint, I can't see how it makes sense to push this
code into hid/hidraw.c when it is bluetooth-specific. Further, I can't
see how it makes sense to do the USB portion the hard way, when the
current implementation is so compact and non-error-prone.
Clients to hidraw provide two functions with very simple interfaces, one
for outputting reports, and one for getting (requesting and receiving)
reports. I think having clean interfaces between modules has a lot of value.
All that said, I'm always open to better ideas. Maybe you have a better
design idea that you can enlighten me with.
Alan.
^ permalink raw reply
* Re: [PATCH] at1700: fix double free_irq
From: Dan Carpenter @ 2010-07-09 18:56 UTC (permalink / raw)
To: Kulikov Vasiliy
Cc: kernel-janitors, David S. Miller, Jiri Pirko, Joe Perches,
Stephen Hemminger, Eric Dumazet, netdev
In-Reply-To: <20100709174809.GA5228@albatros>
On Fri, Jul 09, 2010 at 09:48:09PM +0400, Kulikov Vasiliy wrote:
> > > /* Disable the IRQ on boards of fmv18x where it is feasible. */
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > It seems like this comment should be updated?
>
> Maybe yes, but I don't know what these damn IO requests mean.
> Sure, it's better to request IRQ in xxx_open(), but as it is already
> done in probe() I leave it here.
>
> If it is a bug then I do nothing with it, but if it is not then I'll
> create a bug.
>
Yeah. I see what you mean. You are probably right. It should be easy
to test if anyone had the hardware. But this driver is 17 years old so
I doubt anyone does. :P
regards,
dan carpenter
^ permalink raw reply
* [PATCH net-next-2.6 1/2] net: Get rid of rtnl_link_stats64 / net_device_stats union
From: Ben Hutchings @ 2010-07-09 19:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers, Eric Dumazet
In commit be1f3c2c027cc5ad735df6a45a542ed1db7ec48b "net: Enable 64-bit
net device statistics on 32-bit architectures" I redefined struct
net_device_stats so that it could be used in a union with struct
rtnl_link_stats64, avoiding the need for explicit copying or
conversion between the two. However, this is unsafe because there is
no locking required and no lock consistently held around calls to
dev_get_stats() and use of the statistics structure it returns.
In commit 28172739f0a276eb8d6ca917b3974c2edb036da3 "net: fix 64 bit
counters on 32 bit arches" Eric Dumazet dealt with that problem by
requiring callers of dev_get_stats() to provide storage for the
result. This means that the net_device::stats64 field and the padding
in struct net_device_stats are now redundant, so remove them.
Update the comment on net_device_ops::ndo_get_stats64 to reflect its
new usage.
Change dev_txq_stats_fold() to use struct rtnl_link_stats64, since
that is what all its callers are really using and it is no longer
going to be compatible with struct net_device_stats.
Eric Dumazet suggested the separate function for the structure
conversion.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
drivers/net/macvlan.c | 2 +-
include/linux/netdevice.h | 70 ++++++++++++++++++---------------------------
net/8021q/vlan_dev.c | 2 +-
net/core/dev.c | 31 ++++++++++++++++---
4 files changed, 56 insertions(+), 49 deletions(-)
diff --git a/drivers/net/macvlan.c b/drivers/net/macvlan.c
index 6112f14..1b28aae 100644
--- a/drivers/net/macvlan.c
+++ b/drivers/net/macvlan.c
@@ -436,7 +436,7 @@ static struct rtnl_link_stats64 *macvlan_dev_get_stats64(struct net_device *dev,
{
struct macvlan_dev *vlan = netdev_priv(dev);
- dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
+ dev_txq_stats_fold(dev, stats);
if (vlan->rx_stats) {
struct macvlan_rx_stats *p, accum = {0};
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 8018f6b..17e95e3 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -162,42 +162,32 @@ static inline bool dev_xmit_complete(int rc)
/*
* Old network device statistics. Fields are native words
* (unsigned long) so they can be read and written atomically.
- * Each field is padded to 64 bits for compatibility with
- * rtnl_link_stats64.
*/
-#if BITS_PER_LONG == 64
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long name
-#elif defined(__LITTLE_ENDIAN)
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long name, pad_ ## name
-#else
-#define NET_DEVICE_STATS_DEFINE(name) unsigned long pad_ ## name, name
-#endif
-
struct net_device_stats {
- NET_DEVICE_STATS_DEFINE(rx_packets);
- NET_DEVICE_STATS_DEFINE(tx_packets);
- NET_DEVICE_STATS_DEFINE(rx_bytes);
- NET_DEVICE_STATS_DEFINE(tx_bytes);
- NET_DEVICE_STATS_DEFINE(rx_errors);
- NET_DEVICE_STATS_DEFINE(tx_errors);
- NET_DEVICE_STATS_DEFINE(rx_dropped);
- NET_DEVICE_STATS_DEFINE(tx_dropped);
- NET_DEVICE_STATS_DEFINE(multicast);
- NET_DEVICE_STATS_DEFINE(collisions);
- NET_DEVICE_STATS_DEFINE(rx_length_errors);
- NET_DEVICE_STATS_DEFINE(rx_over_errors);
- NET_DEVICE_STATS_DEFINE(rx_crc_errors);
- NET_DEVICE_STATS_DEFINE(rx_frame_errors);
- NET_DEVICE_STATS_DEFINE(rx_fifo_errors);
- NET_DEVICE_STATS_DEFINE(rx_missed_errors);
- NET_DEVICE_STATS_DEFINE(tx_aborted_errors);
- NET_DEVICE_STATS_DEFINE(tx_carrier_errors);
- NET_DEVICE_STATS_DEFINE(tx_fifo_errors);
- NET_DEVICE_STATS_DEFINE(tx_heartbeat_errors);
- NET_DEVICE_STATS_DEFINE(tx_window_errors);
- NET_DEVICE_STATS_DEFINE(rx_compressed);
- NET_DEVICE_STATS_DEFINE(tx_compressed);
+ unsigned long rx_packets;
+ unsigned long tx_packets;
+ unsigned long rx_bytes;
+ unsigned long tx_bytes;
+ unsigned long rx_errors;
+ unsigned long tx_errors;
+ unsigned long rx_dropped;
+ unsigned long tx_dropped;
+ unsigned long multicast;
+ unsigned long collisions;
+ unsigned long rx_length_errors;
+ unsigned long rx_over_errors;
+ unsigned long rx_crc_errors;
+ unsigned long rx_frame_errors;
+ unsigned long rx_fifo_errors;
+ unsigned long rx_missed_errors;
+ unsigned long tx_aborted_errors;
+ unsigned long tx_carrier_errors;
+ unsigned long tx_fifo_errors;
+ unsigned long tx_heartbeat_errors;
+ unsigned long tx_window_errors;
+ unsigned long rx_compressed;
+ unsigned long tx_compressed;
};
#endif /* __KERNEL__ */
@@ -666,14 +656,13 @@ struct netdev_rx_queue {
* Callback uses when the transmitter has not made any progress
* for dev->watchdog ticks.
*
- * struct rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev
+ * struct rtnl_link_stats64* (*ndo_get_stats64)(struct net_device *dev,
* struct rtnl_link_stats64 *storage);
* struct net_device_stats* (*ndo_get_stats)(struct net_device *dev);
* Called when a user wants to get the network device usage
* statistics. Drivers must do one of the following:
- * 1. Define @ndo_get_stats64 to update a rtnl_link_stats64 structure
- * (which should normally be dev->stats64) and return a ponter to
- * it. The structure must not be changed asynchronously.
+ * 1. Define @ndo_get_stats64 to fill in a zero-initialised
+ * rtnl_link_stats64 structure passed by the caller.
* 2. Define @ndo_get_stats to update a net_device_stats structure
* (which should normally be dev->stats) and return a pointer to
* it. The structure may be changed asynchronously only if each
@@ -888,10 +877,7 @@ struct net_device {
int ifindex;
int iflink;
- union {
- struct rtnl_link_stats64 stats64;
- struct net_device_stats stats;
- };
+ struct net_device_stats stats;
#ifdef CONFIG_WIRELESS_EXT
/* List of functions to handle Wireless Extensions (instead of ioctl).
@@ -2147,7 +2133,7 @@ extern void dev_mcast_init(void);
extern const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
struct rtnl_link_stats64 *storage);
extern void dev_txq_stats_fold(const struct net_device *dev,
- struct net_device_stats *stats);
+ struct rtnl_link_stats64 *stats);
extern int netdev_max_backlog;
extern int netdev_tstamp_prequeue;
diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index a1b8171..7cb285f 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -805,7 +805,7 @@ static u32 vlan_ethtool_get_flags(struct net_device *dev)
static struct rtnl_link_stats64 *vlan_dev_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
{
- dev_txq_stats_fold(dev, (struct net_device_stats *)stats);
+ dev_txq_stats_fold(dev, stats);
if (vlan_dev_info(dev)->vlan_rx_stats) {
struct vlan_rx_stats *p, accum = {0};
diff --git a/net/core/dev.c b/net/core/dev.c
index eb4201c..79ee26e 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5274,10 +5274,10 @@ void netdev_run_todo(void)
/**
* dev_txq_stats_fold - fold tx_queues stats
* @dev: device to get statistics from
- * @stats: struct net_device_stats to hold results
+ * @stats: struct rtnl_link_stats64 to hold results
*/
void dev_txq_stats_fold(const struct net_device *dev,
- struct net_device_stats *stats)
+ struct rtnl_link_stats64 *stats)
{
unsigned long tx_bytes = 0, tx_packets = 0, tx_dropped = 0;
unsigned int i;
@@ -5297,6 +5297,27 @@ void dev_txq_stats_fold(const struct net_device *dev,
}
EXPORT_SYMBOL(dev_txq_stats_fold);
+/* Convert net_device_stats to rtnl_link_stats64. They have the same
+ * fields in the same order, with only the type differing.
+ */
+static void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
+ const struct net_device_stats *netdev_stats)
+{
+#if BITS_PER_LONG == 64
+ BUILD_BUG_ON(sizeof(*stats64) != sizeof(*netdev_stats));
+ memcpy(stats64, netdev_stats, sizeof(*stats64));
+#else
+ size_t i, n = sizeof(*stats64) / sizeof(u64);
+ const unsigned long *src = (const unsigned long *)netdev_stats;
+ u64 *dst = (u64 *)stats64;
+
+ BUILD_BUG_ON(sizeof(*netdev_stats) / sizeof(unsigned long) !=
+ sizeof(*stats64) / sizeof(u64));
+ for (i = 0; i < n; i++)
+ dst[i] = src[i];
+#endif
+}
+
/**
* dev_get_stats - get network device statistics
* @dev: device to get statistics from
@@ -5317,11 +5338,11 @@ const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
return ops->ndo_get_stats64(dev, storage);
}
if (ops->ndo_get_stats) {
- memcpy(storage, ops->ndo_get_stats(dev), sizeof(*storage));
+ netdev_stats_to_stats64(storage, ops->ndo_get_stats(dev));
return storage;
}
- memcpy(storage, &dev->stats, sizeof(*storage));
- dev_txq_stats_fold(dev, (struct net_device_stats *)storage);
+ netdev_stats_to_stats64(storage, &dev->stats);
+ dev_txq_stats_fold(dev, storage);
return storage;
}
EXPORT_SYMBOL(dev_get_stats);
--
1.6.2.5
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
* [PATCH net-next-2.6 2/2] net: Document that dev_get_stats() returns the given pointer
From: Ben Hutchings @ 2010-07-09 19:12 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-net-drivers, Eric Dumazet
In-Reply-To: <1278702713.2078.13.camel@achroite.uk.solarflarecom.com>
Document that dev_get_stats() returns the same stats pointer it was
given. Remove const qualification from the returned pointer since the
caller may do what it likes with that structure.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
include/linux/netdevice.h | 4 ++--
net/core/dev.c | 12 ++++++------
2 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 17e95e3..c4fedf0 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2130,8 +2130,8 @@ extern void netdev_features_change(struct net_device *dev);
/* Load a device via the kmod */
extern void dev_load(struct net *net, const char *name);
extern void dev_mcast_init(void);
-extern const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
- struct rtnl_link_stats64 *storage);
+extern struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
+ struct rtnl_link_stats64 *storage);
extern void dev_txq_stats_fold(const struct net_device *dev,
struct rtnl_link_stats64 *stats);
diff --git a/net/core/dev.c b/net/core/dev.c
index 79ee26e..e2b9fa2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5323,13 +5323,13 @@ static void netdev_stats_to_stats64(struct rtnl_link_stats64 *stats64,
* @dev: device to get statistics from
* @storage: place to store stats
*
- * Get network statistics from device. The device driver may provide
- * its own method by setting dev->netdev_ops->get_stats64 or
- * dev->netdev_ops->get_stats; otherwise the internal statistics
- * structure is used.
+ * Get network statistics from device. Return @storage.
+ * The device driver may provide its own method by setting
+ * dev->netdev_ops->get_stats64 or dev->netdev_ops->get_stats;
+ * otherwise the internal statistics structure is used.
*/
-const struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
- struct rtnl_link_stats64 *storage)
+struct rtnl_link_stats64 *dev_get_stats(struct net_device *dev,
+ struct rtnl_link_stats64 *storage)
{
const struct net_device_ops *ops = dev->netdev_ops;
--
1.6.2.5
--
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply related
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