* Re: [RFC net] bridge: clean the nf_bridge status when forwarding the skb
From: Stephen Hemminger @ 2013-10-14 22:27 UTC (permalink / raw)
To: Antonio Quartulli
Cc: David S. Miller, bridge@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20131014222010.GB3873@neomailbox.net>
On Tue, 15 Oct 2013 00:20:10 +0200
Antonio Quartulli <antonio@meshcoding.com> wrote:
> On Fri, Sep 27, 2013 at 12:01:43AM +0200, Antonio Quartulli wrote:
> > On Thu, Sep 26, 2013 at 02:32:48PM -0700, Stephen Hemminger wrote:
> > > > > > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > > > > > index 4b81b14..65864bc 100644
> > > > > > --- a/net/bridge/br_forward.c
> > > > > > +++ b/net/bridge/br_forward.c
> > > > > > @@ -49,6 +49,11 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
> > > > > > } else {
> > > > > > skb_push(skb, ETH_HLEN);
> > > > > > br_drop_fake_rtable(skb);
> > > > > > +
> > > > > > + /* clean the NF bridge data */
> > > > > > + nf_bridge_put(skb->nf_bridge);
> > > > > > + skb->nf_bridge = NULL;
> > > > > > +
> > > > > > dev_queue_xmit(skb);
> > > > > > }
> > > > > >
> > > >
> > > > Regarding CONFIG_BRIDGE_NETFILTER you are right, thanks.
> > > >
> > > > >
> > > > > I think the header will also be garbage if bridge on bridge with netfilter is used.
> > > > > See nf_bridge_save_header.
> > > >
> > > > What header are you referring to? nf_bridge_save_header() saves the header in
> > > > skb->nf_bridge->data, which is freed during nf_bridge_put() (assuming
> > > > ->use reached 0).
> > > >
> > > >
> > >
> > > If bridge is stacked the original ether header will get overwritten by the second
> > > call to save_header.
> >
> > Sorry, but I am not getting what you mean (I am new to the code and it is late here..):
> > save_header() will store the Ethernet header in nf_bridge->data for
> > later recover (if needed).
> >
> > By freeing nf_bridge I also destroy this header copy.
> >
> > When the skb enters the second bridge, save_header() will save again the header
> > in nf_bridge->data. But I don't see how this can create a problem.
> >
> > The problem I had before this patch comes from the fact that
> > nf_bridge_copy_header() is invoked in the second bridge with the nf_bridge state
> > of the first. This was overwriting my the packet Ethernet header with what the
> > first invocation of save_header() stored in nf_bridge->data.
> >
> > But by unsetting nf_bridge I think I am preventing this from happening again.
> > no?
>
> Hello Stephen,
>
> do you have other comments about this patch? I know it is rather difficult that
> a generic user hits this issue, but I'd like to see it fixed because other
> people using batman-adv may incur in this problem.
>
> Cheers,
>
>
The patch content is fine, but it needs necessary ifdef's.
I would also prefer to have the clean done as a function that can be stubbed out
like the other bridge netfilter stuff.
^ permalink raw reply
* Re: [RFC net] bridge: clean the nf_bridge status when forwarding the skb
From: Antonio Quartulli @ 2013-10-14 22:20 UTC (permalink / raw)
To: Stephen Hemminger
Cc: David S. Miller, bridge@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20130926220143.GC1228@open-mesh.com>
[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]
On Fri, Sep 27, 2013 at 12:01:43AM +0200, Antonio Quartulli wrote:
> On Thu, Sep 26, 2013 at 02:32:48PM -0700, Stephen Hemminger wrote:
> > > > > diff --git a/net/bridge/br_forward.c b/net/bridge/br_forward.c
> > > > > index 4b81b14..65864bc 100644
> > > > > --- a/net/bridge/br_forward.c
> > > > > +++ b/net/bridge/br_forward.c
> > > > > @@ -49,6 +49,11 @@ int br_dev_queue_push_xmit(struct sk_buff *skb)
> > > > > } else {
> > > > > skb_push(skb, ETH_HLEN);
> > > > > br_drop_fake_rtable(skb);
> > > > > +
> > > > > + /* clean the NF bridge data */
> > > > > + nf_bridge_put(skb->nf_bridge);
> > > > > + skb->nf_bridge = NULL;
> > > > > +
> > > > > dev_queue_xmit(skb);
> > > > > }
> > > > >
> > >
> > > Regarding CONFIG_BRIDGE_NETFILTER you are right, thanks.
> > >
> > > >
> > > > I think the header will also be garbage if bridge on bridge with netfilter is used.
> > > > See nf_bridge_save_header.
> > >
> > > What header are you referring to? nf_bridge_save_header() saves the header in
> > > skb->nf_bridge->data, which is freed during nf_bridge_put() (assuming
> > > ->use reached 0).
> > >
> > >
> >
> > If bridge is stacked the original ether header will get overwritten by the second
> > call to save_header.
>
> Sorry, but I am not getting what you mean (I am new to the code and it is late here..):
> save_header() will store the Ethernet header in nf_bridge->data for
> later recover (if needed).
>
> By freeing nf_bridge I also destroy this header copy.
>
> When the skb enters the second bridge, save_header() will save again the header
> in nf_bridge->data. But I don't see how this can create a problem.
>
> The problem I had before this patch comes from the fact that
> nf_bridge_copy_header() is invoked in the second bridge with the nf_bridge state
> of the first. This was overwriting my the packet Ethernet header with what the
> first invocation of save_header() stored in nf_bridge->data.
>
> But by unsetting nf_bridge I think I am preventing this from happening again.
> no?
Hello Stephen,
do you have other comments about this patch? I know it is rather difficult that
a generic user hits this issue, but I'd like to see it fixed because other
people using batman-adv may incur in this problem.
Cheers,
--
Antonio Quartulli
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] iproute2 - Fix tc stats when using -batch mode
From: Nigel Kukard @ 2013-10-14 22:16 UTC (permalink / raw)
To: netdev
[-- Attachment #1.1: Type: text/plain, Size: 528 bytes --]
Hi,
There are two global variables in tc/tc_class.c:
__u32 filter_qdisc;
__u32 filter_classid;
These are not re-initialized for each line received in -batch mode:
# tc -force -batch -
class show dev eth0 parent 1: classid 1:1
class show dev eth0 parent 1: classid 1:1
Error: duplicate "classid": "1:1" is the second value.
This patch fixes the issue by initializing the two globals when we
enter print_class().
Attached a patch which fixes this by initializing them when
print_class() is called.
-N
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: batchfix.patch --]
[-- Type: text/x-patch; name="batchfix.patch", Size: 986 bytes --]
commit d04acab5a8c0267610e9ea21f8a42e54ed4585dc
Author: Nigel Kukard <nkukard@lbsd.net>
Date: Mon Oct 14 22:10:16 2013 +0000
Fix tc stats when using -batch mode
There are two global variables in tc/tc_class.c:
__u32 filter_qdisc;
__u32 filter_classid;
These are not re-initialized for each line received in -batch mode:
class show dev eth0 parent 1: classid 1:1
class show dev eth0 parent 1: classid 1:1
Error: duplicate "classid": "1:1" is the second value.
This patch fixes the issue by initializing the two globals when we
enter print_class().
diff --git a/tc/tc_class.c b/tc/tc_class.c
index 9d4eea5..8043448 100644
--- a/tc/tc_class.c
+++ b/tc/tc_class.c
@@ -241,6 +241,9 @@ int tc_class_list(int argc, char **argv)
t.tcm_family = AF_UNSPEC;
memset(d, 0, sizeof(d));
+ filter_qdisc = 0;
+ filter_classid = 0;
+
while (argc > 0) {
if (strcmp(*argv, "dev") == 0) {
NEXT_ARG();
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4052 bytes --]
^ permalink raw reply related
* Re: kernel policy routing table src ip not respected since 2.6.37 and commit 9fc3bbb4a752
From: Vincent Li @ 2013-10-14 22:12 UTC (permalink / raw)
To: netdev@vger.kernel.org; +Cc: Joel Sing
In-Reply-To: <CAK3+h2zGFnbC-hpPB8fPX7TC3rKCWbPYVY4WW4HDYbN+LXNXXw@mail.gmail.com>
here is the actual test output
[root@centos64-vm linux]# ip addr show dev eth0
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc pfifo_fast
state UP qlen 1000
link/ether 52:54:00:c4:bd:08 brd ff:ff:ff:ff:ff:ff
inet 10.1.1.1/24 scope global eth0 <======
inet 10.1.1.2/24 scope global secondary eth0 <======
inet6 fe80::5054:ff:fec4:bd08/64 scope link
valid_lft forever preferred_lft forever
[root@centos64-vm linux]# ip rule list
0: from all lookup local
245: from all lookup 245
32766: from all lookup main
32767: from all lookup default
[root@centos64-vm linux]# ip route list table 245
10.1.1.0/24 dev eth0 proto kernel scope link src 10.1.1.2 <====use
src 10.1.1.2
[root@centos64-vm linux]# ping 10.1.1.9
PING 10.1.1.9 (10.1.1.9) 56(84) bytes of data.
>From 10.1.1.1 icmp_seq=2 Destination Host Unreachable <==expecting
10.1.1.2 as before 2.6.37, but see 10.1.1.1
>From 10.1.1.1 icmp_seq=3 Destination Host Unreachable
Vincent
On Mon, Oct 14, 2013 at 9:13 AM, Vincent Li <vincent.mc.li@gmail.com> wrote:
> I had a simple bash script to test if the policy routing table src ip
> is respected or not, git bisect found the commit 9fc3bbb4a752 to
> change the policy routing table source ip behavior.
>
> commit 9fc3bbb4a752f108cf096d96640f3b548bbbce6c
> Author: Joel Sing <jsing@google.com>
> Date: Mon Jan 3 20:24:20 2011 +0000
>
> ipv4/route.c: respect prefsrc for local routes
>
> The preferred source address is currently ignored for local routes,
> which results in all local connections having a src address that is the
> same as the local dst address. Fix this by respecting the preferred source
> address when it is provided for local routes.
>
> test script:
>
> #!/bin/bash
> ip addr add 10.1.1.1/24 dev eth0
> ip addr add 10.1.1.2/24 dev eth0
> ip rule add priority 245 table 245
> ip route add 10.1.1.0/24 dev eth0 proto kernel scope link src
> 10.1.1.2 table 245 <===source ip 10.1.1.2 to be preferred
>
> ip addr show dev eth0
> ip route list table main
> ip route list table 245
>
>
> tcpdump -nn -i eth0 host 10.1.1.9 and icmp &
>
> ping 10.1.1.9
>
>
>
> --before commit 9fc3bbb4a752
>
> the source is from ip 10.1.1.2 as expected
>
> --after commit 9fc3bbb4a752
>
> the source is from ip 10.1.1.1 which not expected since I have high
> priority table 245 with source ip 10.1.1.2
>
> is this regression of commit 9fc3bbb4a752 ?
>
> Vincent
^ permalink raw reply
* iproute2/tc and major/minor limits
From: Nigel Kukard @ 2013-10-14 21:45 UTC (permalink / raw)
To: netdev
Hi guys,
I'm just curious if there is any specific reason why 32-bit integers are
used with tc imposing the limit of 0xfff maximum major and minor numbers
for classes? (more specifically architectural or algorithmic reasons)
-N
^ permalink raw reply
* Re: [RFC] bridge and friends: reduce TheLinuxWay(tm)
From: Stephen Hemminger @ 2013-10-14 21:41 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Stephen Hemminger, Vlad Yasevich, netdev@vger.kernel.org
In-Reply-To: <525C62D5.3090004@mojatatu.com>
On Mon, 14 Oct 2013 17:32:05 -0400
Jamal Hadi Salim <jhs@mojatatu.com> wrote:
> IOW, TheCutNpasteTrain.
>
> There's a lot of clutter on the netlink interface used
> by bridge and vxlan.
> 1) A lot of things which are boolean on/off end up using a uchar.
> Example:
> IFLA_BRPORT_MODE, IFLA_BRPORT_GUARD, IFLA_BRPORT_PROTECT,
> IFLA_BRPORT_LEARNING, IFLA_BRPORT_UNICAST_FLOOD, BRIDGE_VLAN_INFO_PVID,
> BRIDGE_VLAN_INFO_UNTAGGED, BRIDGE_MODE_VEPA,
> IFLA_VXLAN_PROXY,IFLA_VXLAN_RSC, etc
>
> 2) There's a few fields which are basically intended to project the
> same message to the kernel but are redefined a few times:
> Example:
> BRIDGE_VLAN_INFO_MASTER vs NTF_MASTER vs BRIDGE_FLAGS_MASTER
>
> Also i am not sure why multicast snooping needs its own subheader.
>
> Is it too late to make changes? git logs shows some of these feature
> have only been on the last 2-3 months.
> One approach to resolve this is introduce a new BRIDGE_FLAGS TLV
> which will work like the ifi_flags/change and put all these flags in
> one spot. New iproute will use these and the old one will continue using
> the old approach. It would require to EOL the old interface at some
> point.
Unfortunately, by now this is all set in ABI.
It was a side effect of the per-feature evolutionary style of development.
^ permalink raw reply
* [RFC] bridge and friends: reduce TheLinuxWay(tm)
From: Jamal Hadi Salim @ 2013-10-14 21:32 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: Vlad Yasevich, netdev@vger.kernel.org
IOW, TheCutNpasteTrain.
There's a lot of clutter on the netlink interface used
by bridge and vxlan.
1) A lot of things which are boolean on/off end up using a uchar.
Example:
IFLA_BRPORT_MODE, IFLA_BRPORT_GUARD, IFLA_BRPORT_PROTECT,
IFLA_BRPORT_LEARNING, IFLA_BRPORT_UNICAST_FLOOD, BRIDGE_VLAN_INFO_PVID,
BRIDGE_VLAN_INFO_UNTAGGED, BRIDGE_MODE_VEPA,
IFLA_VXLAN_PROXY,IFLA_VXLAN_RSC, etc
2) There's a few fields which are basically intended to project the
same message to the kernel but are redefined a few times:
Example:
BRIDGE_VLAN_INFO_MASTER vs NTF_MASTER vs BRIDGE_FLAGS_MASTER
Also i am not sure why multicast snooping needs its own subheader.
Is it too late to make changes? git logs shows some of these feature
have only been on the last 2-3 months.
One approach to resolve this is introduce a new BRIDGE_FLAGS TLV
which will work like the ifi_flags/change and put all these flags in
one spot. New iproute will use these and the old one will continue using
the old approach. It would require to EOL the old interface at some
point.
If this is agreeable and it is not on someone todo list - I will send
patches later on.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH -next] netdev: inet_timewait_sock.h missing semi-colon when KMEMCHECK is enabled
From: Joe Perches @ 2013-10-14 21:25 UTC (permalink / raw)
To: Randy Dunlap
Cc: Thierry Reding, linux-next, linux-kernel, Mark Brown,
netdev@vger.kernel.org, David Miller
In-Reply-To: <525C5863.1000505@infradead.org>
On Mon, 2013-10-14 at 13:47 -0700, Randy Dunlap wrote:
> On 10/14/13 12:53, Joe Perches wrote:
> > On Mon, 2013-10-14 at 12:36 -0700, Randy Dunlap wrote:
> >> From: Randy Dunlap <rdunlap@infradead.org>
> >>
> >> Fix (a few hundred) build errors due to missing semi-colon when
> >> KMEMCHECK is enabled:
keyword: enabled...
> > Shouldn't this be done in kmemcheck.h?
>
> This patch makes sense and probably should be merged
> but it does not fix the build errors that I reported.
Duh, enabled. Yeah, you're sure...
^ permalink raw reply
* Re: [PATCH -next] netdev: inet_timewait_sock.h missing semi-colon when KMEMCHECK is enabled
From: Joe Perches @ 2013-10-14 21:24 UTC (permalink / raw)
To: Randy Dunlap
Cc: Thierry Reding, linux-next, linux-kernel, Mark Brown,
netdev@vger.kernel.org, David Miller
In-Reply-To: <525C5863.1000505@infradead.org>
On Mon, 2013-10-14 at 13:47 -0700, Randy Dunlap wrote:
> On 10/14/13 12:53, Joe Perches wrote:
> > On Mon, 2013-10-14 at 12:36 -0700, Randy Dunlap wrote:
> >> From: Randy Dunlap <rdunlap@infradead.org>
> >>
> >> Fix (a few hundred) build errors due to missing semi-colon when
> >> KMEMCHECK is enabled:
> >>
> >> include/net/inet_timewait_sock.h:139:2: error: expected ',', ';' or '}' before 'int'
> >> include/net/inet_timewait_sock.h:148:28: error: 'const struct inet_timewait_sock' has no member named 'tw_death_node'
> >>
> >> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> >> ---
> >> include/net/inet_timewait_sock.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> --- next-2013-1014.orig/include/net/inet_timewait_sock.h
> >> +++ next-2013-1014/include/net/inet_timewait_sock.h
> >> @@ -135,7 +135,7 @@ struct inet_timewait_sock {
> >> tw_transparent : 1,
> >> tw_pad : 6, /* 6 bits hole */
> >> tw_tos : 8,
> >> - tw_pad2 : 16 /* 16 bits hole */
> >> + tw_pad2 : 16; /* 16 bits hole */
> >> kmemcheck_bitfield_end(flags);
> >> u32 tw_ttd;
> >> struct inet_bind_bucket *tw_tb;
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> Please read the FAQ at http://www.tux.org/lkml/
> >
> > Shouldn't this be done in kmemcheck.h?
>
> This patch makes sense and probably should be merged
> but it does not fix the build errors that I reported.
You sure?
^ permalink raw reply
* [PATCH] net bridge: remove unused field
From: Jamal Hadi Salim @ 2013-10-14 21:11 UTC (permalink / raw)
To: David Miller; +Cc: Stephen Hemminger, netdev@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 29 bytes --]
Trivial patch
cheers,
jamal
[-- Attachment #2: patch-1 --]
[-- Type: text/plain, Size: 649 bytes --]
commit 984b1bc7c51a1201707238f7aa5fa1a8b72fde90
Author: jamal <jhs@mojatatu.com>
Date: Mon Oct 14 17:04:16 2013 -0400
Remove unused enum
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 80394e8..f8dab16 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -209,11 +209,6 @@ enum {
#define IFLA_INET6_MAX (__IFLA_INET6_MAX - 1)
enum {
- BRIDGE_MODE_UNSPEC,
- BRIDGE_MODE_HAIRPIN,
-};
-
-enum {
IFLA_BRPORT_UNSPEC,
IFLA_BRPORT_STATE, /* Spanning tree state */
IFLA_BRPORT_PRIORITY, /* " priority */
^ permalink raw reply related
* Re: SW csum errors
From: Stephen Hemminger @ 2013-10-14 20:58 UTC (permalink / raw)
To: Kyle Hubert; +Cc: netdev
In-Reply-To: <CAJoZ4U3yCrGW=5TO5z0FL2HQ+thZVzVfiPOTxTGKBuR=njadOg@mail.gmail.com>
On Mon, 14 Oct 2013 16:13:15 -0400
Kyle Hubert <khubert@gmail.com> wrote:
> My problem is rather specific. I am working on an RDMA device, and we
> have full end to end reliability. However, one of the initial spins of
> our chip had some errors, since fixed, where the csum was unreliable.
> So, we did exactly what Dave Miller warned not to do in the linked
> message. We ran outgoing IP packets through the SKB checksum
> function.. Unfortunately, we occasionally saw NFS csum errors on full
> MTU packets.
>
> Here is his response:
>
> http://marc.info/?l=linux-netdev&m=128286758300676&w=2
>
> Relevant portion:
>
> "
> Paged SKBs can have references to page cache pages and similar. These
> can be updated asynchronously to the transmit, there is no locking at
> all to freeze the contents, and therefore full checksum offload is
> required to support SG correctly.
>
> So don't get the idea to do the checksum in software in the infiniband
> layer, and advertize hw checksumming support, to get around this :-)
> "
>
> Now that those chips have long gone, I am left pondering about these
> packets "corrupted" before the device transfers them. Can I get more
> information about these paged SKBs with asynchronous modifications?
> How does NFS use them?
You would have to either mark the pages as copy on write or copy the data.
Setting COW is expensive because you have to coordinate with other CPU's
on SMP. Not sure exactly how.
You can demonstrate this with either sendfile() or NFS where underlying
file contents are being modified while packet is in the queue.
^ permalink raw reply
* [PATCH net-next] net: Delete trailing semi-colon from definition of netdev_WARN()
From: Ben Hutchings @ 2013-10-14 20:49 UTC (permalink / raw)
To: David Miller; +Cc: netdev
Macro definitions should not normally end with a semi-colon, as this
makes it dangerous to use them an if...else statement. Happily this
has not happened yet.
Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
include/linux/netdevice.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 2e53b44..27f62f7 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3049,7 +3049,7 @@ do { \
* file/line information and a backtrace.
*/
#define netdev_WARN(dev, format, args...) \
- WARN(1, "netdevice: %s\n" format, netdev_name(dev), ##args);
+ WARN(1, "netdevice: %s\n" format, netdev_name(dev), ##args)
/* netif printk helpers, similar to netdev_printk */
--
Ben Hutchings, Staff Engineer, Solarflare
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
* Re: [PATCH -next] netdev: inet_timewait_sock.h missing semi-colon when KMEMCHECK is enabled
From: Randy Dunlap @ 2013-10-14 20:47 UTC (permalink / raw)
To: Joe Perches
Cc: Thierry Reding, linux-next, linux-kernel, Mark Brown,
netdev@vger.kernel.org, David Miller
In-Reply-To: <1381780437.12919.3.camel@joe-AO722>
On 10/14/13 12:53, Joe Perches wrote:
> On Mon, 2013-10-14 at 12:36 -0700, Randy Dunlap wrote:
>> From: Randy Dunlap <rdunlap@infradead.org>
>>
>> Fix (a few hundred) build errors due to missing semi-colon when
>> KMEMCHECK is enabled:
>>
>> include/net/inet_timewait_sock.h:139:2: error: expected ',', ';' or '}' before 'int'
>> include/net/inet_timewait_sock.h:148:28: error: 'const struct inet_timewait_sock' has no member named 'tw_death_node'
>>
>> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
>> ---
>> include/net/inet_timewait_sock.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> --- next-2013-1014.orig/include/net/inet_timewait_sock.h
>> +++ next-2013-1014/include/net/inet_timewait_sock.h
>> @@ -135,7 +135,7 @@ struct inet_timewait_sock {
>> tw_transparent : 1,
>> tw_pad : 6, /* 6 bits hole */
>> tw_tos : 8,
>> - tw_pad2 : 16 /* 16 bits hole */
>> + tw_pad2 : 16; /* 16 bits hole */
>> kmemcheck_bitfield_end(flags);
>> u32 tw_ttd;
>> struct inet_bind_bucket *tw_tb;
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at http://www.tux.org/lkml/
>
> Shouldn't this be done in kmemcheck.h?
This patch makes sense and probably should be merged
but it does not fix the build errors that I reported.
> include/linux/kmemcheck.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/kmemcheck.h b/include/linux/kmemcheck.h
> index 39f8453..b9ffad5 100644
> --- a/include/linux/kmemcheck.h
> +++ b/include/linux/kmemcheck.h
> @@ -62,10 +62,10 @@ bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size);
> * kmemcheck_annotate_bitfield(a, flags);
> */
> #define kmemcheck_bitfield_begin(name) \
> - int name##_begin[0];
> + int name##_begin[0]
>
> #define kmemcheck_bitfield_end(name) \
> - int name##_end[0];
> + int name##_end[0]
>
> #define kmemcheck_annotate_bitfield(ptr, name) \
> do { \
>
>
> --
--
~Randy
^ permalink raw reply
* Re: SW csum errors
From: Eric Dumazet @ 2013-10-14 20:40 UTC (permalink / raw)
To: Kyle Hubert; +Cc: netdev
In-Reply-To: <CAJoZ4U3yCrGW=5TO5z0FL2HQ+thZVzVfiPOTxTGKBuR=njadOg@mail.gmail.com>
On Mon, 2013-10-14 at 16:13 -0400, Kyle Hubert wrote:
> My problem is rather specific. I am working on an RDMA device, and we
> have full end to end reliability. However, one of the initial spins of
> our chip had some errors, since fixed, where the csum was unreliable.
> So, we did exactly what Dave Miller warned not to do in the linked
> message. We ran outgoing IP packets through the SKB checksum
> function.. Unfortunately, we occasionally saw NFS csum errors on full
> MTU packets.
>
> Here is his response:
>
> http://marc.info/?l=linux-netdev&m=128286758300676&w=2
>
> Relevant portion:
>
> "
> Paged SKBs can have references to page cache pages and similar. These
> can be updated asynchronously to the transmit, there is no locking at
> all to freeze the contents, and therefore full checksum offload is
> required to support SG correctly.
>
> So don't get the idea to do the checksum in software in the infiniband
> layer, and advertize hw checksumming support, to get around this :-)
> "
>
> Now that those chips have long gone, I am left pondering about these
> packets "corrupted" before the device transfers them. Can I get more
> information about these paged SKBs with asynchronous modifications?
> How does NFS use them?
For a start try :
git grep -n SKBTX_SHARED_FRAG
git grep -n skb_has_shared_frag
git show cef401de7be8c4e
git show c9af6db4c11ccc6
^ permalink raw reply
* [PATCH net-next] 8390 ei_debug : Reenable the use of debugging in 8390 based chips
From: Matthew Whitehead @ 2013-10-14 20:33 UTC (permalink / raw)
To: netdev; +Cc: Matthew Whitehead
Ethernet boards based on the 8390 chip had an '#ifdef notdef' disabling the
use of the debug variable ei_debug. Reenable it for those of us who still
occasionally use it.
Signed-off-by: Matthew Whitehead <tedheadster@gmail.com>
---
drivers/net/ethernet/8390/8390.h | 2 +-
drivers/net/ethernet/8390/axnet_cs.c | 5 -----
drivers/net/ethernet/8390/lib8390.c | 4 ++--
3 files changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/8390/8390.h b/drivers/net/ethernet/8390/8390.h
index ef325ff..2375087 100644
--- a/drivers/net/ethernet/8390/8390.h
+++ b/drivers/net/ethernet/8390/8390.h
@@ -21,7 +21,7 @@ struct e8390_pkt_hdr {
unsigned short count; /* header + packet length in bytes */
};
-#ifdef notdef
+#ifdef EI_DEBUG
extern int ei_debug;
#else
#define ei_debug 1
diff --git a/drivers/net/ethernet/8390/axnet_cs.c b/drivers/net/ethernet/8390/axnet_cs.c
index d801c141..581560c 100644
--- a/drivers/net/ethernet/8390/axnet_cs.c
+++ b/drivers/net/ethernet/8390/axnet_cs.c
@@ -810,11 +810,6 @@ module_pcmcia_driver(axnet_cs_driver);
#define ei_block_input (ei_local->block_input)
#define ei_get_8390_hdr (ei_local->get_8390_hdr)
-/* use 0 for production, 1 for verification, >2 for debug */
-#ifndef ei_debug
-int ei_debug = 1;
-#endif
-
/* Index to functions. */
static void ei_tx_intr(struct net_device *dev);
static void ei_tx_err(struct net_device *dev);
diff --git a/drivers/net/ethernet/8390/lib8390.c b/drivers/net/ethernet/8390/lib8390.c
index b329f5c..4ec5e38 100644
--- a/drivers/net/ethernet/8390/lib8390.c
+++ b/drivers/net/ethernet/8390/lib8390.c
@@ -100,8 +100,8 @@
#define ei_get_8390_hdr (ei_local->get_8390_hdr)
/* use 0 for production, 1 for verification, >2 for debug */
-#ifndef ei_debug
-int ei_debug = 1;
+#ifdef EI_DEBUG
+int ei_debug = EI_DEBUG;
#endif
/* Index to functions. */
--
1.7.2.5
^ permalink raw reply related
* SW csum errors
From: Kyle Hubert @ 2013-10-14 20:13 UTC (permalink / raw)
To: netdev
My problem is rather specific. I am working on an RDMA device, and we
have full end to end reliability. However, one of the initial spins of
our chip had some errors, since fixed, where the csum was unreliable.
So, we did exactly what Dave Miller warned not to do in the linked
message. We ran outgoing IP packets through the SKB checksum
function.. Unfortunately, we occasionally saw NFS csum errors on full
MTU packets.
Here is his response:
http://marc.info/?l=linux-netdev&m=128286758300676&w=2
Relevant portion:
"
Paged SKBs can have references to page cache pages and similar. These
can be updated asynchronously to the transmit, there is no locking at
all to freeze the contents, and therefore full checksum offload is
required to support SG correctly.
So don't get the idea to do the checksum in software in the infiniband
layer, and advertize hw checksumming support, to get around this :-)
"
Now that those chips have long gone, I am left pondering about these
packets "corrupted" before the device transfers them. Can I get more
information about these paged SKBs with asynchronous modifications?
How does NFS use them?
Thanks for your time,
-Kyle
^ permalink raw reply
* RE: INVESTMENT/ RELOCATION ASSISTANCE. 14th/10/2013
From: Mrs. Maryann Jamila Hussein. @ 2013-10-14 19:55 UTC (permalink / raw)
To: Recipients
Dear Beloved,
I am Mrs. Maryann Jamila Hussein a Teacher and a Muslim Convert here in Syria,i had sent a previous mail which i am not sure you got. I need your assistance to invest and help me relocate my 3 kids who are 17 years and below, so that they can get a better life there in your country due to the on going crises here in Syria.
I need your trust, before the death of my husband we had a savings with an Indian Bank, so money is not the issue.
I got your reference in my search for someone who suits my
purpose.If you can help me reply, let me know.
Regards,
Mrs. Maryann Jamila Hussein.
=====================================
^ permalink raw reply
* Re: [PATCH -next] netdev: inet_timewait_sock.h missing semi-colon when KMEMCHECK is enabled
From: Joe Perches @ 2013-10-14 19:53 UTC (permalink / raw)
To: Randy Dunlap
Cc: Thierry Reding, linux-next, linux-kernel, Mark Brown,
netdev@vger.kernel.org, David Miller
In-Reply-To: <525C47C0.2000907@infradead.org>
On Mon, 2013-10-14 at 12:36 -0700, Randy Dunlap wrote:
> From: Randy Dunlap <rdunlap@infradead.org>
>
> Fix (a few hundred) build errors due to missing semi-colon when
> KMEMCHECK is enabled:
>
> include/net/inet_timewait_sock.h:139:2: error: expected ',', ';' or '}' before 'int'
> include/net/inet_timewait_sock.h:148:28: error: 'const struct inet_timewait_sock' has no member named 'tw_death_node'
>
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
> ---
> include/net/inet_timewait_sock.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- next-2013-1014.orig/include/net/inet_timewait_sock.h
> +++ next-2013-1014/include/net/inet_timewait_sock.h
> @@ -135,7 +135,7 @@ struct inet_timewait_sock {
> tw_transparent : 1,
> tw_pad : 6, /* 6 bits hole */
> tw_tos : 8,
> - tw_pad2 : 16 /* 16 bits hole */
> + tw_pad2 : 16; /* 16 bits hole */
> kmemcheck_bitfield_end(flags);
> u32 tw_ttd;
> struct inet_bind_bucket *tw_tb;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
Shouldn't this be done in kmemcheck.h?
include/linux/kmemcheck.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/linux/kmemcheck.h b/include/linux/kmemcheck.h
index 39f8453..b9ffad5 100644
--- a/include/linux/kmemcheck.h
+++ b/include/linux/kmemcheck.h
@@ -62,10 +62,10 @@ bool kmemcheck_is_obj_initialized(unsigned long addr, size_t size);
* kmemcheck_annotate_bitfield(a, flags);
*/
#define kmemcheck_bitfield_begin(name) \
- int name##_begin[0];
+ int name##_begin[0]
#define kmemcheck_bitfield_end(name) \
- int name##_end[0];
+ int name##_end[0]
#define kmemcheck_annotate_bitfield(ptr, name) \
do { \
^ permalink raw reply related
* Re: [PATCH 01/07] 8139too: Support for byte queue limits
From: Tino Reichardt @ 2013-10-14 19:52 UTC (permalink / raw)
To: netdev
In-Reply-To: <1381777891.2045.1.camel@edumazet-glaptop.roam.corp.google.com>
* Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2013-10-14 at 20:26 +0200, Tino Reichardt wrote:
> > Changes to 8139too to use byte queue limits.
> >
> > @@ -1733,6 +1735,7 @@ static netdev_tx_t rtl8139_start_xmit (struct sk_buff *skb,
> > tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
> >
> > tp->cur_tx++;
> > + netdev_sent_queue(dev, len);
> >
>
> This looks wrong if len < ETH_ZLEN
Fixed this (really stupid) issue, updated patch is located here:
http://www.mcmilk.de/projects/linux-bql/dl/0001-8139too-Support-for-byte-queue-limits.patch
The netif_dbg() statement will now also print the correct queued length.
--
Best regards, TR
^ permalink raw reply
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set
From: Aaro Koskinen @ 2013-10-14 19:49 UTC (permalink / raw)
To: Dan Carpenter
Cc: support, David Daney, Greg KH, driverdev-devel,
Sebastian Pöhn, netdev
In-Reply-To: <20131014191541.GA11797@mwanda>
On Mon, Oct 14, 2013 at 10:16:49PM +0300, Dan Carpenter wrote:
> On Mon, Oct 14, 2013 at 09:39:06PM +0300, Aaro Koskinen wrote:
> > It's initialized in cvm_oct_phy_setup_device():
> >
> > priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
> ^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
>
> Sorry I should have explained better.
>
> We use cvm_oct_adjust_link() to initialize priv->phydev but
> cvm_oct_adjust_link() depends on priv->phydev. It seems like we would
> hit the NULL dereference every time. Weird huh?
It doesn't happen on my system (EdgeRouter Lite). I think you need to
explain even more better. :-)
What you mean by "We use cvm_oct_adjust_link() to initialize
priv->phydev..."? Sorry, maybe I'm just missing something really
obvious...
A.
^ permalink raw reply
* [PATCH -next] netdev: inet_timewait_sock.h missing semi-colon when KMEMCHECK is enabled
From: Randy Dunlap @ 2013-10-14 19:36 UTC (permalink / raw)
To: Thierry Reding, linux-next, linux-kernel
Cc: Mark Brown, netdev@vger.kernel.org, David Miller
In-Reply-To: <1381762088-18880-1-git-send-email-treding@nvidia.com>
From: Randy Dunlap <rdunlap@infradead.org>
Fix (a few hundred) build errors due to missing semi-colon when
KMEMCHECK is enabled:
include/net/inet_timewait_sock.h:139:2: error: expected ',', ';' or '}' before 'int'
include/net/inet_timewait_sock.h:148:28: error: 'const struct inet_timewait_sock' has no member named 'tw_death_node'
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
---
include/net/inet_timewait_sock.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
--- next-2013-1014.orig/include/net/inet_timewait_sock.h
+++ next-2013-1014/include/net/inet_timewait_sock.h
@@ -135,7 +135,7 @@ struct inet_timewait_sock {
tw_transparent : 1,
tw_pad : 6, /* 6 bits hole */
tw_tos : 8,
- tw_pad2 : 16 /* 16 bits hole */
+ tw_pad2 : 16; /* 16 bits hole */
kmemcheck_bitfield_end(flags);
u32 tw_ttd;
struct inet_bind_bucket *tw_tb;
^ permalink raw reply
* Re: [PATCH 01/07] 8139too: Support for byte queue limits
From: Tino Reichardt @ 2013-10-14 19:31 UTC (permalink / raw)
To: netdev
In-Reply-To: <1381777891.2045.1.camel@edumazet-glaptop.roam.corp.google.com>
* Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Mon, 2013-10-14 at 20:26 +0200, Tino Reichardt wrote:
> > Changes to 8139too to use byte queue limits.
> >
> > This patch was not tested on real hardware currently, but compiles fine and
> > should work.
> >
> > tp->cur_tx = 0;
> > tp->dirty_tx = 0;
> > + netdev_reset_queue(tp->dev);
> >
> > /* XXX account for unsent Tx packets in tp->stats.tx_dropped */
> > }
> > @@ -1733,6 +1735,7 @@ static netdev_tx_t rtl8139_start_xmit (struct sk_buff *skb,
> > tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
> >
> > tp->cur_tx++;
> > + netdev_sent_queue(dev, len);
> >
>
> This looks wrong if len < ETH_ZLEN
Yes you are right. I looked to the debug statement in the end of that
function, so len had to be correct ... but it isn't ... yes :(
netif_dbg(tp, tx_queued, dev, "Queued Tx packet size %u to slot %d\n"...
Should a make len = max(len, ETH_ZLEN) there .. so the debug statement
is also correct?
--
Best regards, TR
^ permalink raw reply
* Re: [PATCH] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set
From: Dan Carpenter @ 2013-10-14 19:16 UTC (permalink / raw)
To: Aaro Koskinen
Cc: support, David Daney, Greg KH, driverdev-devel,
Sebastian Pöhn, netdev
In-Reply-To: <20131014183906.GA4260@blackmetal.musicnaut.iki.fi>
On Mon, Oct 14, 2013 at 09:39:06PM +0300, Aaro Koskinen wrote:
> Hi,
>
> On Mon, Oct 14, 2013 at 01:10:51PM +0300, Dan Carpenter wrote:
> > On Sun, Oct 13, 2013 at 02:28:10PM -0700, Greg KH wrote:
> > > On Sun, Oct 13, 2013 at 08:59:54PM +0200, Sebastian Pöhn wrote:
> > > > A zero pointer deref on priv->phydev->link was causing oops on our systems.
> > > > Might be related to improper configuration but we should fail gracefully here ;-)
> > > >
> > > > Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com>
> > > >
> > > > ---
> > > >
> > > > diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
> > > > index 83b1030..bc8c503 100644
> > > > --- a/drivers/staging/octeon/ethernet-mdio.c
> > > > +++ b/drivers/staging/octeon/ethernet-mdio.c
> > > > @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev)
> > > > struct octeon_ethernet *priv = netdev_priv(dev);
> > > > cvmx_helper_link_info_t link_info;
> > > >
> > > > + if(!priv->phydev)
> > > > + return ;
> > >
> > > Please always run your patches through the scripts/checkpatch.pl tool so
> > > that maintainers don't have to point out the obvious coding syle errors
> > > by hand each time :)
> >
> > Also it's whitespace damaged and doesn't apply.
> >
> > >
> > > Care to try again?
> > >
> > > Also, how was phydev NULL? What was causing that?
> >
> > To me it looks like phydev is always NULL.
>
> It's initialized in cvm_oct_phy_setup_device():
>
> priv->phydev = of_phy_connect(dev, phy_node, cvm_oct_adjust_link, 0,
^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^^^
Sorry I should have explained better.
We use cvm_oct_adjust_link() to initialize priv->phydev but
cvm_oct_adjust_link() depends on priv->phydev. It seems like we would
hit the NULL dereference every time. Weird huh?
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH 01/07] 8139too: Support for byte queue limits
From: Eric Dumazet @ 2013-10-14 19:11 UTC (permalink / raw)
To: Tino Reichardt
Cc: netdev, David S. Miller, Joe Perches, Jiri Pirko, Bill Pemberton,
Greg Kroah-Hartman
In-Reply-To: <1381775183-24866-2-git-send-email-milky-kernel@mcmilk.de>
On Mon, 2013-10-14 at 20:26 +0200, Tino Reichardt wrote:
> Changes to 8139too to use byte queue limits.
>
> This patch was not tested on real hardware currently, but compiles fine and
> should work.
>
>
> Signed-off-by: Tino Reichardt <milky-kernel@mcmilk.de>
>
> ---
> drivers/net/ethernet/realtek/8139too.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/ethernet/realtek/8139too.c b/drivers/net/ethernet/realtek/8139too.c
> index 3ccedeb..992ac57 100644
> --- a/drivers/net/ethernet/realtek/8139too.c
> +++ b/drivers/net/ethernet/realtek/8139too.c
> @@ -1409,6 +1409,7 @@ static void rtl8139_hw_start (struct net_device *dev)
> }
>
> netdev_dbg(dev, "init buffer addresses\n");
> + netdev_reset_queue(dev);
>
> /* Lock Config[01234] and BMCR register writes */
> RTL_W8 (Cfg9346, Cfg9346_Lock);
> @@ -1638,6 +1639,7 @@ static inline void rtl8139_tx_clear (struct rtl8139_private *tp)
> {
> tp->cur_tx = 0;
> tp->dirty_tx = 0;
> + netdev_reset_queue(tp->dev);
>
> /* XXX account for unsent Tx packets in tp->stats.tx_dropped */
> }
> @@ -1733,6 +1735,7 @@ static netdev_tx_t rtl8139_start_xmit (struct sk_buff *skb,
> tp->tx_flag | max(len, (unsigned int)ETH_ZLEN));
>
> tp->cur_tx++;
> + netdev_sent_queue(dev, len);
>
This looks wrong if len < ETH_ZLEN
> if ((tp->cur_tx - NUM_TX_DESC) == tp->dirty_tx)
> netif_stop_queue (dev);
> @@ -1750,6 +1753,7 @@ static void rtl8139_tx_interrupt (struct net_device *dev,
> void __iomem *ioaddr)
> {
> unsigned long dirty_tx, tx_left;
> + unsigned bytes_compl = 0, pkts_compl = 0;
>
> assert (dev != NULL);
> assert (ioaddr != NULL);
> @@ -1792,6 +1796,8 @@ static void rtl8139_tx_interrupt (struct net_device *dev,
> u64_stats_update_begin(&tp->tx_stats.syncp);
> tp->tx_stats.packets++;
> tp->tx_stats.bytes += txstatus & 0x7ff;
> + pkts_compl++;
> + bytes_compl += txstatus & 0x7ff;
Because here len reported by NIC will be >= ETH_ZLEN
^ permalink raw reply
* Re: [PATCHv2] staging: octeon-ethernet: trivial: Avoid OOPS if phydev is not set
From: Aaro Koskinen @ 2013-10-14 19:06 UTC (permalink / raw)
To: Sebastian Pöhn
Cc: driverdev-devel@linuxdriverproject.org, netdev@vger.kernel.org,
dan.carpenter
In-Reply-To: <1381773535.2049.4.camel@alpha.Speedport_W723_V_Typ_A_1_00_098>
On Mon, Oct 14, 2013 at 07:58:55PM +0200, Sebastian Pöhn wrote:
> Sorry. Haven't signed off for a while now :(
>
> I bet that this is really an issue of incorrect OF information. If I find out more I'll let you know.
>
> @dan: The code works for some interfaces - so phydev is set correctly in some cases.
Kernel git log is not a chat forum; please write a proper changelog,
if possible include the original oops log.
Thanks,
A.
> Signed-off-by: Sebastian Poehn <sebastian.poehn@googlemail.com>
> ---
> diff --git a/drivers/staging/octeon/ethernet-mdio.c b/drivers/staging/octeon/ethernet-mdio.c
> index 83b1030..bc8c503 100644
> --- a/drivers/staging/octeon/ethernet-mdio.c
> +++ b/drivers/staging/octeon/ethernet-mdio.c
> @@ -121,6 +121,9 @@ static void cvm_oct_adjust_link(struct net_device *dev)
> struct octeon_ethernet *priv = netdev_priv(dev);
> cvmx_helper_link_info_t link_info;
>
> + if (!priv->phydev)
> + return;
> +
> if (priv->last_link != priv->phydev->link) {
> priv->last_link = priv->phydev->link;
> link_info.u64 = 0;
>
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ 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