* [PATCH] PPC: bpf_jit_comp: add SKF_AD_PKTTYPE instruction
From: Denis Kirjanov @ 2014-10-26 19:23 UTC (permalink / raw)
To: netdev; +Cc: linuxppc-dev, Denis Kirjanov, Matt Evans
Cc: Matt Evans <matt@ozlabs.org>
Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
---
arch/powerpc/include/asm/ppc-opcode.h | 1 +
arch/powerpc/net/bpf_jit.h | 7 +++++++
arch/powerpc/net/bpf_jit_comp.c | 5 +++++
3 files changed, 13 insertions(+)
diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 6f85362..1a52877 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -204,6 +204,7 @@
#define PPC_INST_ERATSX_DOT 0x7c000127
/* Misc instructions for BPF compiler */
+#define PPC_INST_LBZ 0x88000000
#define PPC_INST_LD 0xe8000000
#define PPC_INST_LHZ 0xa0000000
#define PPC_INST_LHBRX 0x7c00062c
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 9aee27c..c406aa9 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -87,6 +87,9 @@ DECLARE_LOAD_FUNC(sk_load_byte_msh);
#define PPC_STD(r, base, i) EMIT(PPC_INST_STD | ___PPC_RS(r) | \
___PPC_RA(base) | ((i) & 0xfffc))
+
+#define PPC_LBZ(r, base, i) EMIT(PPC_INST_LBZ | ___PPC_RT(r) | \
+ ___PPC_RA(base) | IMM_L(i))
#define PPC_LD(r, base, i) EMIT(PPC_INST_LD | ___PPC_RT(r) | \
___PPC_RA(base) | IMM_L(i))
#define PPC_LWZ(r, base, i) EMIT(PPC_INST_LWZ | ___PPC_RT(r) | \
@@ -96,6 +99,10 @@ DECLARE_LOAD_FUNC(sk_load_byte_msh);
#define PPC_LHBRX(r, base, b) EMIT(PPC_INST_LHBRX | ___PPC_RT(r) | \
___PPC_RA(base) | ___PPC_RB(b))
/* Convenience helpers for the above with 'far' offsets: */
+#define PPC_LBZ_OFFS(r, base, i) do { if ((i) < 32768) PPC_LBZ(r, base, i); \
+ else { PPC_ADDIS(r, base, IMM_HA(i)); \
+ PPC_LBZ(r, r, IMM_L(i)); } } while(0)
+
#define PPC_LD_OFFS(r, base, i) do { if ((i) < 32768) PPC_LD(r, base, i); \
else { PPC_ADDIS(r, base, IMM_HA(i)); \
PPC_LD(r, r, IMM_L(i)); } } while(0)
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index cbae2df..d110e28 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -407,6 +407,11 @@ static int bpf_jit_build_body(struct bpf_prog *fp, u32 *image,
PPC_LHZ_OFFS(r_A, r_skb, offsetof(struct sk_buff,
queue_mapping));
break;
+ case BPF_ANC | SKF_AD_PKTTYPE:
+ PPC_LBZ_OFFS(r_A, r_skb, PKT_TYPE_OFFSET());
+ PPC_ANDI(r_A, r_A, PKT_TYPE_MAX);
+ PPC_SRWI(r_A, r_A, 5);
+ break;
case BPF_ANC | SKF_AD_CPU:
#ifdef CONFIG_SMP
/*
--
2.1.0
^ permalink raw reply related
* Re: some failures with vxlan offloads..
From: Or Gerlitz @ 2014-10-26 22:23 UTC (permalink / raw)
To: Tom Herbert
Cc: Or Gerlitz, netdev@vger.kernel.org, John Fastabend, Jeff Kirsher
In-Reply-To: <CA+mtBx-Jp5Kps5WycYwN0PuyXLDtGM0s8USxdQoSLPRDVS19xw@mail.gmail.com>
On Sun, Oct 26, 2014 at 5:29 PM, Tom Herbert <therbert@google.com> wrote:
> On Sun, Oct 26, 2014 at 6:36 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
>> In the cases where it breaks I can see
>> UDP: bad checksum. From 192.168.31.18:54748 to 192.168.31.17:4789
>> ulen 726
>> prints from __udp4_lib_rcv() in the kernel log of the node where offloads
>> are OFF, where the bad packet is sent from the host where offloading is
>> enabled. I guess the packet is just dropped:
> Can you determine what the TSO HW engine is setting in UDP checksum
> field? tcpdump -vv might be able to show this. The symptoms seem to
> indicate that it may not be zero.
Thanks for the quick response. I'll check what is placed in the UDP
checksum field for packets that went through the offloading HW and let
you know.
BTW, if following the direction you proposed, I wonder why this works
(e.g the kernel doesn't drops the encapsulated TCP packets) when both
sides are offloaded?
Tomorrow (Monday) I am OOO so will be able to do these further tests Tuesday.
Or.
^ permalink raw reply
* Re: [PATCH] ipv6: notify userspace when we added or changed an ipv6 token
From: Lubomir Rintel @ 2014-10-26 22:28 UTC (permalink / raw)
To: Daniel Borkmann
Cc: netdev, linux-kernel, David S. Miller, Hannes Frederic Sowa
In-Reply-To: <543B9F5A.8070909@redhat.com>
On Mon, 2014-10-13 at 11:46 +0200, Daniel Borkmann wrote:
> On 10/10/2014 04:08 PM, Lubomir Rintel wrote:
> > NetworkManager might want to know that it changed when the router advertisement
> > arrives.
> >
> > Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> > Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Cc: Daniel Borkmann <dborkman@redhat.com>
> > ---
> > net/ipv6/addrconf.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> > index 3e118df..3d11390 100644
> > --- a/net/ipv6/addrconf.c
> > +++ b/net/ipv6/addrconf.c
> > @@ -4528,6 +4528,7 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
> > }
> >
> > write_unlock_bh(&idev->lock);
> > + netdev_state_change(dev);
>
> I'm wondering why netdev_state_change()? You are probably
> only after the netlink notification that is being invoked,
> i.e. rtmsg_ifinfo(RTM_NEWLINK, ...), and don't strictly want
> to call the device notifier chain.
Correct. I'll change it to just rtmsg_ifinfo().
> Perhaps it might be better to define a new RTM_SETTOKEN, and
> just call inet6_ifinfo_notify(RTM_SETTOKEN, idev) as this is
> only idev specific anyway?
I'm not really sure as that would require more userspace changes than
necessary.
> > addrconf_verify_rtnl();
> > return 0;
> > }
> >
^ permalink raw reply
* [PATCH v2] ipv6: notify userspace when we added or changed an ipv6 token
From: Lubomir Rintel @ 2014-10-26 22:41 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, Lubomir Rintel, Hannes Frederic Sowa,
Daniel Borkmann
In-Reply-To: <1412950112-15593-1-git-send-email-lkundrak@v3.sk>
NetworkManager might want to know that it changed when the router advertisement
arrives.
Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
Cc: Daniel Borkmann <dborkman@redhat.com>
---
Changes since v1:
- Do not call device notifier chain with netdev_state_change()
net/ipv6/addrconf.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 3e118df..f6f92f5 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -4528,6 +4528,7 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
}
write_unlock_bh(&idev->lock);
+ rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
addrconf_verify_rtnl();
return 0;
}
--
1.9.3
^ permalink raw reply related
* Re: [PATCH v2] ipv6: notify userspace when we added or changed an ipv6 token
From: Daniel Borkmann @ 2014-10-26 23:22 UTC (permalink / raw)
To: Lubomir Rintel; +Cc: netdev, David S. Miller, Hannes Frederic Sowa
In-Reply-To: <1414363283-31410-1-git-send-email-lkundrak@v3.sk>
On 10/26/2014 11:41 PM, Lubomir Rintel wrote:
> NetworkManager might want to know that it changed when the router advertisement
> arrives.
>
> Signed-off-by: Lubomir Rintel <lkundrak@v3.sk>
> Cc: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Cc: Daniel Borkmann <dborkman@redhat.com>
The reason why I asked regarding the rtmsg_ifinfo() vs inet6_ifinfo_notify()
in v1 is actually that this is an idev-only specific action. By using
inet6_ifinfo_notify() for notification, the kernel would actually need to do
much less work: the notification only needs inet6_fill_ifinfo() as that would
contain the new token, while the rtmsg_ifinfo() is rather dev-centric and
fills out attributes about the _whole_ device (which surely includes the IPv6
idev attributes, but also a lot more, which might actually be unnecessary
here), see also:
$ git grep -n rtmsg_ifinfo net/ipv6/
$ git grep -n inet6_ifinfo_notify net/ipv6/
net/ipv6/addrconf.c:2919: inet6_ifinfo_notify(RTM_NEWLINK, idev);
net/ipv6/addrconf.c:4650:void inet6_ifinfo_notify(int event, struct inet6_dev *idev)
net/ipv6/ndisc.c:1239: inet6_ifinfo_notify(RTM_NEWLINK, in6_dev);
net/ipv6/ndisc.c:1256: inet6_ifinfo_notify(RTM_NEWLINK, in6_dev);
net/ipv6/ndisc.c:1712: inet6_ifinfo_notify(RTM_NEWLINK, idev);
Maybe I'm missing something, so can you elaborate why it's _absolutely not_
possible to use inet6_ifinfo_notify()?
> ---
> Changes since v1:
> - Do not call device notifier chain with netdev_state_change()
>
> net/ipv6/addrconf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 3e118df..f6f92f5 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -4528,6 +4528,7 @@ static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
> }
>
> write_unlock_bh(&idev->lock);
> + rtmsg_ifinfo(RTM_NEWLINK, dev, 0, GFP_KERNEL);
> addrconf_verify_rtnl();
> return 0;
> }
>
^ permalink raw reply
* Re: Fw: [Bug 86851] New: Reproducible panic on heavy UDP traffic
From: Nikolay Aleksandrov @ 2014-10-26 23:28 UTC (permalink / raw)
To: Florian Westphal, Stephen Hemminger; +Cc: netdev
In-Reply-To: <20141025214448.GB28407@breakpoint.cc>
On 10/25/2014 11:44 PM, Florian Westphal wrote:
> Stephen Hemminger <stephen@networkplumber.org> wrote:
>
> [ CC Nik ]
>
>> Date: Fri, 24 Oct 2014 11:34:08 -0700
>> From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
>> To: "stephen@networkplumber.org" <stephen@networkplumber.org>
>> Subject: [Bug 86851] New: Reproducible panic on heavy UDP traffic
>>
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=86851
>>
>> Bug ID: 86851
>> Summary: Reproducible panic on heavy UDP traffic
>> Product: Networking
>> Version: 2.5
>> Kernel Version: 3.18-rc1
>> Hardware: x86-64
>> OS: Linux
>> Tree: Mainline
>> Status: NEW
>> Severity: normal
>> Priority: P1
>> Component: IPV4
>> Assignee: shemminger@linux-foundation.org
>> Reporter: chutzpah@gentoo.org
>> Regression: No
>>
>> Created attachment 154861
>> --> https://bugzilla.kernel.org/attachment.cgi?id=154861&action=edit
>> Panic message captured over serial console
>
> general protection fault: 0000 [#1] SMP
> Modules linked in: nfs [..]
> CPU: 7 PID: 257 Comm: kworker/7:1 Tainted: G W 3.18.0-rc1-base-7+ #2
>
> asked reporter to check if there is a warning before the oops.
>
> Hardware name: Intel Corporation S2600GZ/S2600GZ, BIOS SE5C600.86B.02.03.0003.041920141333 04/19/2014
> Workqueue: events inet_frag_worker
> task: ffff882fd32e70e0 ti: ffff882fd0adc000 task.ti: ffff882fd0adc000
> RIP: 0010:[<ffffffff81592ab4>] [<ffffffff81592ab4>] inet_evict_bucket+0xf4/0x180
> RSP: 0018:ffff882fd0adfd58 EFLAGS: 00010286
> RAX: ffff8817c7230701 RBX: dead000000100100 RCX: 0000000180300013
>
> Hello LIST_POISON!
>
> RDX: 0000000180300014 RSI: 0000000000000001 RDI: dead0000001000c0
> RBP: 0000000000000002 R08: 0000000000000202 R09: ffff88303fc39ab0
> R10: ffffffff81592ac0 R11: ffffea005f1c8c00 R12: ffffffff81aa2820
> R13: ffff882fd0adfd70 R14: ffff8817c72307e0 R15: 0000000000000000
> FS: 0000000000000000(0000) GS:ffff88303fc20000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> device rack0a left promiscuous mode
> CR2: 00007f054c7ba034 CR3: 0000002fc4986000 CR4: 00000000001407e0
> Stack:
> ffffffff81aa3298 ffffffff81aa3290 ffff8817d0820a08 0000000000000000
> 0000000000000000 00000000000000a8 0000000000000008 ffff88303fc32780
> ffffffff81aa6820 0000000000000059[ 2415.026338] device rack1a left promiscuous mode
>
> 0000000000000000 ffffffff81592ba2
> Call Trace:
> [<ffffffff81592ba2>] ? inet_frag_worker+0x62/0x210
> [<ffffffff8112c312>] ? process_one_work+0x132/0x360
> [..]
> crash is in hlist_for_each_entry_safe() at the end of inet_evict_bucket(), looks like
> we encounter an already-list_del'd element while iterating.
>
> Will look at this tomorrow.
>
Thanks for CCing me.
I'll dig in the code tomorrow but my first thought when I saw this was
could it be possible that we have a race condition between
ip_frag_queue() and inet_frag_evict(), more precisely between the
ipq_kill() calls from ip_frag_queue and inet_frag_evict since the frag
could be found before we have entered the evictor which then can add it to
its expire list but the ipq_kill() from ip_frag_queue() can do a list_del
after we release the chain lock in the evictor so we may end up like this ?
^ permalink raw reply
* Re: [PATCH] netlink: don't copy over empty attribute data
From: Sasha Levin @ 2014-10-26 23:32 UTC (permalink / raw)
To: David Miller; +Cc: a.ryabinin, pablo, mschmidt, akpm, linux-kernel, netdev
In-Reply-To: <20141022.021508.2011745433893496421.davem@davemloft.net>
On 10/22/2014 02:15 AM, David Miller wrote:
> From: Sasha Levin <sasha.levin@oracle.com>
> Date: Tue, 21 Oct 2014 22:19:36 -0400
>
>> On 10/21/2014 09:39 PM, David Miller wrote:
>>> From: Sasha Levin <sasha.levin@oracle.com>
>>> Date: Tue, 21 Oct 2014 16:51:09 -0400
>>>
>>>>> netlink uses empty data to seperate different levels. However, we still
>>>>> try to copy that data from a NULL ptr using memcpy, which is an undefined
>>>>> behaviour.
>>>>>
>>>>> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
>>> This isn't a POSIX C library, this it the Linux kernel, and as such
>>> we can make sure none of our memcpy() implementations try to access
>>> any bytes if the given length is NULL.
>>
>> We can make *our* implementations work around that undefined behaviour if we
>> want, but right now our implementations is to call GCC's builtin memcpy(),
>> which follows the standards and doesn't allow you to call it with NULL 'from'
>> ptr.
>>
>> The fact that it doesn't die and behaves properly is just "luck".
>
> If GCC's internal memcpy() starts accessing past 'len', I'm going
> to report the bug rather than code around it.
How so? GCC states clearly that you should *never* pass a NULL pointer there:
"The pointers passed to memmove (and similar functions in <string.h>) must
be non-null even when nbytes==0" (https://gcc.gnu.org/gcc-4.9/porting_to.html).
Even if it doesn't dereference it, it can break somehow in a subtle way. Leaving
the kernel code assuming that gcc (or any other compiler) would always behave
the same in a situation that shouldn't occur.
Thanks,
Sasha
^ permalink raw reply
* [PATCH net] net: phy: Add SGMII Configuration for Marvell 88E1145 Initialization
From: Vince Bridgers @ 2014-10-26 19:22 UTC (permalink / raw)
To: f.fainelli, netdev; +Cc: vbridger, vbridger
Marvell phy 88E1145 configuration & initialization was missing a case
for initializing SGMII mode. This patch adds that case.
Signed-off-by: Vince Bridgers <vbridger@opensource.altera.com>
---
drivers/net/phy/marvell.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bd37e45..225c033 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -50,10 +50,15 @@
#define MII_M1011_PHY_SCR 0x10
#define MII_M1011_PHY_SCR_AUTO_CROSS 0x0060
+#define MII_M1145_PHY_EXT_SR 0x1b
#define MII_M1145_PHY_EXT_CR 0x14
#define MII_M1145_RGMII_RX_DELAY 0x0080
#define MII_M1145_RGMII_TX_DELAY 0x0002
+#define MII_M1145_HWCFG_MODE_SGMII_NO_CLK 0x4
+#define MII_M1145_HWCFG_MODE_MASK 0xf
+#define MII_M1145_HWCFG_FIBER_COPPER_AUTO 0x8000
+
#define MII_M1111_PHY_LED_CONTROL 0x18
#define MII_M1111_PHY_LED_DIRECT 0x4100
#define MII_M1111_PHY_LED_COMBINE 0x411c
@@ -676,6 +681,20 @@ static int m88e1145_config_init(struct phy_device *phydev)
}
}
+ if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+ int temp = phy_read(phydev, MII_M1145_PHY_EXT_SR);
+ if (temp < 0)
+ return temp;
+
+ temp &= ~MII_M1145_HWCFG_MODE_MASK;
+ temp |= MII_M1145_HWCFG_MODE_SGMII_NO_CLK;
+ temp |= MII_M1145_HWCFG_FIBER_COPPER_AUTO;
+
+ err = phy_write(phydev, MII_M1145_PHY_EXT_SR, temp);
+ if (err < 0)
+ return err;
+ }
+
err = marvell_of_reg_init(phydev);
if (err < 0)
return err;
--
1.9.1
^ permalink raw reply related
* Re: Fw: [Bug 86851] New: Reproducible panic on heavy UDP traffic
From: Eric Dumazet @ 2014-10-27 0:47 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: Florian Westphal, Stephen Hemminger, netdev
In-Reply-To: <544D8386.9030609@redhat.com>
On Mon, 2014-10-27 at 00:28 +0100, Nikolay Aleksandrov wrote:
>
> Thanks for CCing me.
> I'll dig in the code tomorrow but my first thought when I saw this was
> could it be possible that we have a race condition between
> ip_frag_queue() and inet_frag_evict(), more precisely between the
> ipq_kill() calls from ip_frag_queue and inet_frag_evict since the frag
> could be found before we have entered the evictor which then can add it to
> its expire list but the ipq_kill() from ip_frag_queue() can do a list_del
> after we release the chain lock in the evictor so we may end up like this ?
Yes, either we use hlist_del_init() but loose poison aid, or test if
frag was evicted :
Not sure about refcount.
diff --git a/net/ipv4/inet_fragment.c b/net/ipv4/inet_fragment.c
index 9eb89f3f0ee4..894ec30c5896 100644
--- a/net/ipv4/inet_fragment.c
+++ b/net/ipv4/inet_fragment.c
@@ -285,7 +285,8 @@ static inline void fq_unlink(struct inet_frag_queue *fq, struct inet_frags *f)
struct inet_frag_bucket *hb;
hb = get_frag_bucket_locked(fq, f);
- hlist_del(&fq->list);
+ if (!(fq->flags & INET_FRAG_EVICTED))
+ hlist_del(&fq->list);
spin_unlock(&hb->chain_lock);
}
^ permalink raw reply related
* Re: [QA-TCP] How to send tcp small packages immediately?
From: Zhangjie (HZ) @ 2014-10-27 1:08 UTC (permalink / raw)
To: Rick Jones, kvm, Jason Wang, Michael S. Tsirkin, linux-kernel,
netdev, liuyongan, qinchuanyu
In-Reply-To: <544A6E12.2000007@hp.com>
Thanks!
On 2014/10/24 23:19, Rick Jones wrote:
> On 10/24/2014 12:41 AM, Zhangjie (HZ) wrote:
>> Hi,
>>
>> I use netperf to test the performance of small tcp package, with TCP_NODELAY set :
>>
>> netperf -H 129.9.7.164 -l 100 -- -m 512 -D
>>
>> Among the packages I got by tcpdump, there is not only small packages, also lost of
>> big ones (skb->len=65160).
>>
>> IP 129.9.7.186.60840 > 129.9.7.164.34607: tcp 65160
>> IP 129.9.7.164.34607 > 129.9.7.186.60840: tcp 0
>> IP 129.9.7.164.34607 > 129.9.7.186.60840: tcp 0
>> IP 129.9.7.164.34607 > 129.9.7.186.60840: tcp 0
>> IP 129.9.7.186.60840 > 129.9.7.164.34607: tcp 65160
>> IP 129.9.7.164.34607 > 129.9.7.186.60840: tcp 0
>> IP 129.9.7.164.34607 > 129.9.7.186.60840: tcp 0
>> IP 129.9.7.164.34607 > 129.9.7.186.60840: tcp 0
>> IP 129.9.7.186.60840 > 129.9.7.164.34607: tcp 80
>> IP 129.9.7.186.60840 > 129.9.7.164.34607: tcp 512
>> IP 129.9.7.186.60840 > 129.9.7.164.34607: tcp 512
>>
>> SO, how to test small tcp packages? Including TCP_NODELAY, What else should be set?
>
> Well, I don't think there is anything else you can set. Even with TCP_NODELAY set, segment size with TCP will still be controlled by factors such as congestion window.
>
> I am ass-u-me-ing your packet trace is at the sender. I suppose if your sender were fast enough compared to the path that might combine with congestion window to result in the very large segments.
>
> Not to say there cannot be a bug somewhere with TSO overriding TCP_NODELAY, but in broad terms, even TCP_NODELAY does not guarantee small TCP segments. That has been something of a bane on my attempts to use TCP for aggregate small-packet performance measurements via netperf for quite some time.
>
> And since you seem to have included a virtualization mailing list I would also ass-u-me that virtualization is involved somehow. Knuth only knows how that will affect the timing of events, which will be very much involved in matters of congestion window and such. I suppose it is even possible that if the packet trace is on a VM receiver that some delays in getting the VM running could mean that GRO would end-up making large segments being pushed up the stack.
>
> happy benchmarking,
Yes. Using netperf to send tcp packages frome physical nic has the same problems.
Thanks for your explanation!
>
> rick jones
> .
>
--
Best Wishes!
Zhang Jie
^ permalink raw reply
* Re: some failures with vxlan offloads..
From: Tom Herbert @ 2014-10-27 1:23 UTC (permalink / raw)
To: Or Gerlitz
Cc: Or Gerlitz, netdev@vger.kernel.org, John Fastabend, Jeff Kirsher
In-Reply-To: <CAJ3xEMi6UuCT+6iriX02qjS6trhEaoYh39UBL+p_i995MvZc2Q@mail.gmail.com>
On Sun, Oct 26, 2014 at 3:23 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Sun, Oct 26, 2014 at 5:29 PM, Tom Herbert <therbert@google.com> wrote:
>> On Sun, Oct 26, 2014 at 6:36 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
>
>>> In the cases where it breaks I can see
>>> UDP: bad checksum. From 192.168.31.18:54748 to 192.168.31.17:4789
>>> ulen 726
>>> prints from __udp4_lib_rcv() in the kernel log of the node where offloads
>>> are OFF, where the bad packet is sent from the host where offloading is
>>> enabled. I guess the packet is just dropped:
>
>> Can you determine what the TSO HW engine is setting in UDP checksum
>> field? tcpdump -vv might be able to show this. The symptoms seem to
>> indicate that it may not be zero.
>
> Thanks for the quick response. I'll check what is placed in the UDP
> checksum field for packets that went through the offloading HW and let
> you know.
>
> BTW, if following the direction you proposed, I wonder why this works
> (e.g the kernel doesn't drops the encapsulated TCP packets) when both
> sides are offloaded?
>
I'm just speculating, but the device may be returning checksum
unnecessary for the UDP checksum without actually checking it.
Technically, VXLAN RFC7348 allows an implementation to ignore the UDP
checksum, although this clearly violates RFC1122 UDP checksum
requirements. In the stack we now checksum all non-zero checksums
including UDP checksum in VXLAN if it's not marked
checksum-unnecessary.
Tom
> Tomorrow (Monday) I am OOO so will be able to do these further tests Tuesday.
>
> Or.
^ permalink raw reply
* Re: [PATCH] netlink: don't copy over empty attribute data
From: David Miller @ 2014-10-27 2:03 UTC (permalink / raw)
To: sasha.levin; +Cc: a.ryabinin, pablo, mschmidt, akpm, linux-kernel, netdev
In-Reply-To: <544D849A.4040304@oracle.com>
From: Sasha Levin <sasha.levin@oracle.com>
Date: Sun, 26 Oct 2014 19:32:42 -0400
> How so? GCC states clearly that you should *never* pass a NULL
> pointer there:
>
> "The pointers passed to memmove (and similar functions in <string.h>) must
> be non-null even when nbytes==0" (https://gcc.gnu.org/gcc-4.9/porting_to.html).
>
> Even if it doesn't dereference it, it can break somehow in a subtle way. Leaving
> the kernel code assuming that gcc (or any other compiler) would always behave
> the same in a situation that shouldn't occur.
Show me a legal way in which one could legally dereference the pointer
when length is zero, and I'll entertain this patch.
^ permalink raw reply
* [PATCH] skbuff.h: fix kernel-doc warning for headers_end
From: Randy Dunlap @ 2014-10-27 2:14 UTC (permalink / raw)
To: netdev@vger.kernel.org, David Miller
From: Randy Dunlap <rdunlap@infradead.org>
Fix kernel-doc warning in <linux/skbuff.h> by making both headers_start
and headers_end private fields.
Warning(..//include/linux/skbuff.h:654): No description found for parameter 'headers_end[0]'
Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
---
include/linux/skbuff.h | 4 ++++
1 file changed, 4 insertions(+)
Or I can document those two fields if you would rather not have them
marked as private...
--- lnx-318-rc2.orig/include/linux/skbuff.h
+++ lnx-318-rc2/include/linux/skbuff.h
@@ -557,7 +557,9 @@ struct sk_buff {
/* fields enclosed in headers_start/headers_end are copied
* using a single memcpy() in __copy_skb_header()
*/
+ /* private: */
__u32 headers_start[0];
+ /* public: */
/* if you move pkt_type around you also must adapt those constants */
#ifdef __BIG_ENDIAN_BITFIELD
@@ -642,7 +644,9 @@ struct sk_buff {
__u16 network_header;
__u16 mac_header;
+ /* private: */
__u32 headers_end[0];
+ /* public: */
/* These elements must be at the end, see alloc_skb() for details. */
sk_buff_data_t tail;
^ permalink raw reply
* Re: [PATCH v2 09/15] net: dsa: Add support for switch EEPROM access
From: Andrew Lunn @ 2014-10-27 2:41 UTC (permalink / raw)
To: Guenter Roeck
Cc: netdev, David S. Miller, Florian Fainelli, Andrew Lunn,
linux-kernel
In-Reply-To: <1414342365-27191-10-git-send-email-linux@roeck-us.net>
On Sun, Oct 26, 2014 at 09:52:39AM -0700, Guenter Roeck wrote:
> On some chips it is possible to access the switch eeprom.
> Add infrastructure support for it.
>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2:
> - Add support for configuring switch EEPROM size through platform data
> or devicetree data
> - Do not compare new pointers against NULL
> - Check if get_eeprom pointer is set before calling it
>
> include/net/dsa.h | 10 ++++++++++
> net/dsa/dsa.c | 4 ++++
> net/dsa/slave.c | 41 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 55 insertions(+)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 55e75e7..37856a2 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -38,6 +38,9 @@ struct dsa_chip_data {
> struct device *host_dev;
> int sw_addr;
>
> + /* set to size of eeprom if supported by the switch */
> + int eeprom_len;
> +
> /* Device tree node pointer for this specific switch chip
> * used during switch setup in case additional properties
> * and resources needs to be used
> @@ -258,6 +261,13 @@ struct dsa_switch_driver {
> int (*set_temp_limit)(struct dsa_switch *ds, int temp);
> int (*get_temp_alarm)(struct dsa_switch *ds, bool *alarm);
> #endif
> +
> + /* EEPROM access */
> + int (*get_eeprom_len)(struct dsa_switch *ds);
> + int (*get_eeprom)(struct dsa_switch *ds,
> + struct ethtool_eeprom *eeprom, u8 *data);
> + int (*set_eeprom)(struct dsa_switch *ds,
> + struct ethtool_eeprom *eeprom, u8 *data);
> };
>
> void register_switch_driver(struct dsa_switch_driver *type);
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index 5edbbca..1c1b925 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -575,6 +575,7 @@ static int dsa_of_probe(struct platform_device *pdev)
> const char *port_name;
> int chip_index, port_index;
> const unsigned int *sw_addr, *port_reg;
> + u32 eeprom_length;
> int ret;
>
> mdio = of_parse_phandle(np, "dsa,mii-bus", 0);
> @@ -603,6 +604,9 @@ static int dsa_of_probe(struct platform_device *pdev)
> if (pd->nr_chips > DSA_MAX_SWITCHES)
> pd->nr_chips = DSA_MAX_SWITCHES;
>
> + if (!of_property_read_u32(np, "eeprom-length", &eeprom_length))
> + pd->eeprom_length = eeprom_length;
> +
Hi Guenter
I would of expected a property to indicate the eeprom is present, not
a length value. The switch determines the length, or at least the
minimum length. So the switch driver can return the length, if the
eeprom is indicated to be present.
Also, all device tree properties need to be documented. I've not
looked through all the patches, so maybe it is in a separate patch?
> +static int dsa_slave_get_eeprom_len(struct net_device *dev)
> +{
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + struct dsa_switch *ds = p->parent;
> +
> + if (ds->pd->eeprom_len)
> + return ds->pd->eeprom_len;
> +
> + if (ds->drv->get_eeprom_len)
> + return ds->drv->get_eeprom_len(ds);
> +
> + return 0;
> +}
This also does not look right. The default is that there is no
property in DT, meaning the EEPROM is not present. So
ds->pd->eeprom_len is zero. So the first if is not taken. It then
calls into the driver which is return a length, independent of if it
is populated or not.
If we are going with your suggested binding, no value in DT, or a
value of 0 mean no EEPROM. If there is a value in DT
dsa_slave_get_eeprom_len() should return that value. Asking the driver
is redundant, you can remove the get_eeprom_len() call.
However, if DT just indicates the eeprom is populated or not, you
should call the driver get_eeprom_len() is the eeprom property is
present.
> +static int dsa_slave_get_eeprom(struct net_device *dev,
> + struct ethtool_eeprom *eeprom, u8 *data)
> +{
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + struct dsa_switch *ds = p->parent;
> +
> + if (ds->drv->get_eeprom)
> + return ds->drv->get_eeprom(ds, eeprom, data);
This should be conditional on the length/present.
> +
> + return -EOPNOTSUPP;
> +}
> +
> +static int dsa_slave_set_eeprom(struct net_device *dev,
> + struct ethtool_eeprom *eeprom, u8 *data)
> +{
> + struct dsa_slave_priv *p = netdev_priv(dev);
> + struct dsa_switch *ds = p->parent;
> +
> + if (ds->drv->set_eeprom)
> + return ds->drv->set_eeprom(ds, eeprom, data);
Same again.
Andrew
> + return -EOPNOTSUPP;
> +}
> +
> static void dsa_slave_get_strings(struct net_device *dev,
> uint32_t stringset, uint8_t *data)
> {
> @@ -387,6 +425,9 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
> .get_drvinfo = dsa_slave_get_drvinfo,
> .nway_reset = dsa_slave_nway_reset,
> .get_link = dsa_slave_get_link,
> + .get_eeprom_len = dsa_slave_get_eeprom_len,
> + .get_eeprom = dsa_slave_get_eeprom,
> + .set_eeprom = dsa_slave_set_eeprom,
> .get_strings = dsa_slave_get_strings,
> .get_ethtool_stats = dsa_slave_get_ethtool_stats,
> .get_sset_count = dsa_slave_get_sset_count,
> --
> 1.9.1
>
^ permalink raw reply
* Re: [PATHC] net: napi_reuse_skb() should check pfmemalloc
From: David Miller @ 2014-10-27 2:47 UTC (permalink / raw)
To: eric.dumazet
Cc: klamm, jeffrey.t.kirsher, jesse.brandeburg, bruce.w.allan,
carolyn.wyborny, donald.c.skidmore, gregory.v.rose,
peter.p.waskiewicz.jr, alexander.h.duyck, john.ronciak,
tushar.n.dave, sassmann, gregkh, e1000-devel, netdev,
linux-kernel
In-Reply-To: <1414071030.2094.46.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 23 Oct 2014 06:30:30 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> Do not reuse skb if it was pfmemalloc tainted, otherwise
> future frame might be dropped anyway.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: IPv6 UFO for VMs
From: Ben Hutchings @ 2014-10-27 3:21 UTC (permalink / raw)
To: Hannes Frederic Sowa; +Cc: netdev, virtualization
In-Reply-To: <1413970541.2337.16.camel@localhost>
[-- Attachment #1: Type: text/plain, Size: 2173 bytes --]
On Wed, 2014-10-22 at 11:35 +0200, Hannes Frederic Sowa wrote:
> On Mi, 2014-10-22 at 00:44 +0100, Ben Hutchings wrote:
> > There are several ways that VMs can take advantage of UFO and get the
> > host to do fragmentation for them:
> >
> > drivers/net/macvtap.c: gso_type = SKB_GSO_UDP;
> > drivers/net/tun.c: skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> > drivers/net/virtio_net.c: skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> >
> > Our implementation of UFO for IPv6 does:
> >
> > fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen);
> > fptr->nexthdr = nexthdr;
> > fptr->reserved = 0;
> > fptr->identification = skb_shinfo(skb)->ip6_frag_id;
> >
> > which assumes ip6_frag_id has been set. That's only true if the local
> > stack constructed the skb; otherwise it appears we get zero.
> >
> > This seems to be a regression as a result of:
> >
> > commit 916e4cf46d0204806c062c8c6c4d1f633852c5b6
> > Author: Hannes Frederic Sowa <hannes@stressinduktion.org>
> > Date: Fri Feb 21 02:55:35 2014 +0100
> >
> > ipv6: reuse ip6_frag_id from ip6_ufo_append_data
> >
> > However, that change seems reasonable - we *shouldn't* be choosing IDs
> > for any other stack. Any paravirt net driver that can use IPv6 UFO
> > needs to have some way of passing a fragmentation ID to put in
> > skb_shared_info::ip6_frag_id.
>
> Do we really gain a lot of performance by enabling UFO on those devices
> or would it make sense to just drop support? It only helps fragmenting
> large UDP packets, so I don't think it is worth it.
It's not been important enough for anyone to bother implementing it in
hardware/firmware aside from Neterion.
I'll shortly post patches to disable it.
Ben.
> Otherwise I agree with Ben, we need to pass a fragmentation id from the
> host over to the system segmenting the gso frame. Fragmentation ids must
> be generated by the end system.
>
> Hmm...
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
^ permalink raw reply
* Re: [PATCH v2 09/15] net: dsa: Add support for switch EEPROM access
From: Guenter Roeck @ 2014-10-27 3:56 UTC (permalink / raw)
To: Andrew Lunn; +Cc: netdev, David S. Miller, Florian Fainelli, linux-kernel
In-Reply-To: <20141027024121.GA16998@lunn.ch>
On 10/26/2014 07:41 PM, Andrew Lunn wrote:
> On Sun, Oct 26, 2014 at 09:52:39AM -0700, Guenter Roeck wrote:
>> On some chips it is possible to access the switch eeprom.
>> Add infrastructure support for it.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2:
>> - Add support for configuring switch EEPROM size through platform data
>> or devicetree data
>> - Do not compare new pointers against NULL
>> - Check if get_eeprom pointer is set before calling it
>>
>> include/net/dsa.h | 10 ++++++++++
>> net/dsa/dsa.c | 4 ++++
>> net/dsa/slave.c | 41 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 55 insertions(+)
>>
>> diff --git a/include/net/dsa.h b/include/net/dsa.h
>> index 55e75e7..37856a2 100644
>> --- a/include/net/dsa.h
>> +++ b/include/net/dsa.h
>> @@ -38,6 +38,9 @@ struct dsa_chip_data {
>> struct device *host_dev;
>> int sw_addr;
>>
>> + /* set to size of eeprom if supported by the switch */
>> + int eeprom_len;
>> +
>> /* Device tree node pointer for this specific switch chip
>> * used during switch setup in case additional properties
>> * and resources needs to be used
>> @@ -258,6 +261,13 @@ struct dsa_switch_driver {
>> int (*set_temp_limit)(struct dsa_switch *ds, int temp);
>> int (*get_temp_alarm)(struct dsa_switch *ds, bool *alarm);
>> #endif
>> +
>> + /* EEPROM access */
>> + int (*get_eeprom_len)(struct dsa_switch *ds);
>> + int (*get_eeprom)(struct dsa_switch *ds,
>> + struct ethtool_eeprom *eeprom, u8 *data);
>> + int (*set_eeprom)(struct dsa_switch *ds,
>> + struct ethtool_eeprom *eeprom, u8 *data);
>> };
>>
>> void register_switch_driver(struct dsa_switch_driver *type);
>> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
>> index 5edbbca..1c1b925 100644
>> --- a/net/dsa/dsa.c
>> +++ b/net/dsa/dsa.c
>> @@ -575,6 +575,7 @@ static int dsa_of_probe(struct platform_device *pdev)
>> const char *port_name;
>> int chip_index, port_index;
>> const unsigned int *sw_addr, *port_reg;
>> + u32 eeprom_length;
>> int ret;
>>
>> mdio = of_parse_phandle(np, "dsa,mii-bus", 0);
>> @@ -603,6 +604,9 @@ static int dsa_of_probe(struct platform_device *pdev)
>> if (pd->nr_chips > DSA_MAX_SWITCHES)
>> pd->nr_chips = DSA_MAX_SWITCHES;
>>
>> + if (!of_property_read_u32(np, "eeprom-length", &eeprom_length))
>> + pd->eeprom_length = eeprom_length;
>> +
>
> Hi Guenter
>
> I would of expected a property to indicate the eeprom is present, not
> a length value. The switch determines the length, or at least the
> minimum length. So the switch driver can return the length, if the
> eeprom is indicated to be present.
>
MV88E6352 and compatible chips can neither provide presence nor
length information, so both have to come from platform data or
devicetree.
> Also, all device tree properties need to be documented. I've not
> looked through all the patches, so maybe it is in a separate patch?
>
The documentation is in the next patch.
I merged both presence and length into one optional property, following
the logic that providing a length implies presence and that presence
without length doesn't really provide the required information.
I also wanted to give drivers a chance to indicate presence and length
without the need of devicetree properties or platform data, if a chip
can provide that information.
Also, the only indication to the calling code (ethtool) that there
is not eeprom is through the length field, so I figured that a separate
"presence" field would, for all practical purpose, be just noise
and not be of practical value.
>> +static int dsa_slave_get_eeprom_len(struct net_device *dev)
>> +{
>> + struct dsa_slave_priv *p = netdev_priv(dev);
>> + struct dsa_switch *ds = p->parent;
>> +
>> + if (ds->pd->eeprom_len)
>> + return ds->pd->eeprom_len;
>> +
>> + if (ds->drv->get_eeprom_len)
>> + return ds->drv->get_eeprom_len(ds);
>> +
>> + return 0;
>> +}
>
> This also does not look right. The default is that there is no
> property in DT, meaning the EEPROM is not present. So
> ds->pd->eeprom_len is zero. So the first if is not taken. It then
> calls into the driver which is return a length, independent of if it
> is populated or not.
>
No property was supposed to mean that the information about presence
and length is not provided through devicetree or platform data,
but must be provided by the driver. Devicetree and platform data were
meant to augment auto-detection of presence and length, not to replace
it.
> If we are going with your suggested binding, no value in DT, or a
> value of 0 mean no EEPROM. If there is a value in DT
> dsa_slave_get_eeprom_len() should return that value. Asking the driver
> is redundant, you can remove the get_eeprom_len() call.
>
Again, the MV chips can not provide length or presence information.
That was the whole point of having platform / devicetree data.
> However, if DT just indicates the eeprom is populated or not, you
> should call the driver get_eeprom_len() is the eeprom property is
> present.
>
Guess I was trying to be too intelligent.
So lets take a step back: For the Marvell chips, I have to provide both length
and presence in devicetree or platform data. Presence seemed to be implied by
length, so I used only a single property and variable to indicate both.
Are you asking me to make that mandatory, ie to have presence and length
_always_ be provided by devicetree or platform data ? Sure, I can do that
and simply drop the get_length dsa callback function.
Also, it seems that you request two separate properties, one for presence
and another for length. Is that correct ? Again, I thought that would not
provide any value since presence is indicated by length != 0 in the ethtool
callback function. No problem, though, I'll be happy to create two separate
properties and platform data variables if you think that would be better.
>> +static int dsa_slave_get_eeprom(struct net_device *dev,
>> + struct ethtool_eeprom *eeprom, u8 *data)
>> +{
>> + struct dsa_slave_priv *p = netdev_priv(dev);
>> + struct dsa_switch *ds = p->parent;
>> +
>> + if (ds->drv->get_eeprom)
>> + return ds->drv->get_eeprom(ds, eeprom, data);
>
> This should be conditional on the length/present.
>
>> +
>> + return -EOPNOTSUPP;
>> +}
>> +
>> +static int dsa_slave_set_eeprom(struct net_device *dev,
>> + struct ethtool_eeprom *eeprom, u8 *data)
>> +{
>> + struct dsa_slave_priv *p = netdev_priv(dev);
>> + struct dsa_switch *ds = p->parent;
>> +
>> + if (ds->drv->set_eeprom)
>> + return ds->drv->set_eeprom(ds, eeprom, data);
>
> Same again.
>
> Andrew
>
Sure, no problem.
Thanks,
Guenter
>
>> + return -EOPNOTSUPP;
>> +}
>> +
>> static void dsa_slave_get_strings(struct net_device *dev,
>> uint32_t stringset, uint8_t *data)
>> {
>> @@ -387,6 +425,9 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
>> .get_drvinfo = dsa_slave_get_drvinfo,
>> .nway_reset = dsa_slave_nway_reset,
>> .get_link = dsa_slave_get_link,
>> + .get_eeprom_len = dsa_slave_get_eeprom_len,
>> + .get_eeprom = dsa_slave_get_eeprom,
>> + .set_eeprom = dsa_slave_set_eeprom,
>> .get_strings = dsa_slave_get_strings,
>> .get_ethtool_stats = dsa_slave_get_ethtool_stats,
>> .get_sset_count = dsa_slave_get_sset_count,
>> --
>> 1.9.1
>>
>
^ permalink raw reply
* [PATCH net 0/2] drivers/net,ipv6: Fix IPv6 fragment ID selection for virtio
From: Ben Hutchings @ 2014-10-27 4:17 UTC (permalink / raw)
To: netdev; +Cc: Hannes Frederic Sowa, virtualization
[-- Attachment #1.1: Type: text/plain, Size: 855 bytes --]
The virtio net protocol supports UFO but does not provide for passing a
fragment ID for fragmentation of IPv6 packets. We used to generate a
fragment ID wherever such a packet was fragmented, but currently we
always use ID=0!
Ben.
Ben Hutchings (2):
drivers/net: Disable UFO through virtio
drivers/net,ipv6: Select IPv6 fragment idents for virtio UFO packets
drivers/net/macvtap.c | 16 ++++++++--------
drivers/net/tun.c | 24 +++++++++++++++---------
drivers/net/virtio_net.c | 23 +++++++++++++----------
include/net/ipv6.h | 2 ++
net/ipv6/output_core.c | 34 ++++++++++++++++++++++++++++++++++
5 files changed, 72 insertions(+), 27 deletions(-)
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH net 1/2] drivers/net: Disable UFO through virtio
From: Ben Hutchings @ 2014-10-27 4:22 UTC (permalink / raw)
To: netdev; +Cc: Hannes Frederic Sowa, virtualization
In-Reply-To: <1414383425.16849.18.camel@decadent.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 8525 bytes --]
IPv6 does not allow fragmentation by routers, so there is no
fragmentation ID in the fixed header. UFO for IPv6 requires the ID to
be passed separately, but there is no provision for this in the virtio
net protocol.
Until recently our software implementation of UFO/IPv6 generated a new
ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet
passed through a tap, which is even worse.
Unfortunately there is no distinction between UFO/IPv4 and v6
features, so disable UFO on taps and virtio_net completely until we
have a proper solution.
We cannot depend on VM managers respecting the tap feature flags, so
keep accepting UFO packets but log a warning the first time we do
this.
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Fixes: 916e4cf46d02 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data")
---
I hoped that turning off the UFO feature completely would work, but when
I used libvirt and QEMU on Debian stable I found they still advertised
UFO to the guest.
Ben.
drivers/net/macvtap.c | 13 +++++--------
drivers/net/tun.c | 18 ++++++++++--------
drivers/net/virtio_net.c | 23 +++++++++++++----------
3 files changed, 28 insertions(+), 26 deletions(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 65e2892..2aeaa61 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -65,7 +65,7 @@ static struct cdev macvtap_cdev;
static const struct proto_ops macvtap_socket_ops;
#define TUN_OFFLOADS (NETIF_F_HW_CSUM | NETIF_F_TSO_ECN | NETIF_F_TSO | \
- NETIF_F_TSO6 | NETIF_F_UFO)
+ NETIF_F_TSO6)
#define RX_OFFLOADS (NETIF_F_GRO | NETIF_F_LRO)
#define TAP_FEATURES (NETIF_F_GSO | NETIF_F_SG)
@@ -569,6 +569,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
+ current->comm);
gso_type = SKB_GSO_UDP;
break;
default:
@@ -614,8 +616,6 @@ static void macvtap_skb_to_vnet_hdr(const struct sk_buff *skb,
vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo->gso_type & SKB_GSO_TCPV6)
vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
- else if (sinfo->gso_type & SKB_GSO_UDP)
- vnet_hdr->gso_type = VIRTIO_NET_HDR_GSO_UDP;
else
BUG();
if (sinfo->gso_type & SKB_GSO_TCP_ECN)
@@ -950,9 +950,6 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
if (arg & TUN_F_TSO6)
feature_mask |= NETIF_F_TSO6;
}
-
- if (arg & TUN_F_UFO)
- feature_mask |= NETIF_F_UFO;
}
/* tun/tap driver inverts the usage for TSO offloads, where
@@ -963,7 +960,7 @@ static int set_offload(struct macvtap_queue *q, unsigned long arg)
* When user space turns off TSO, we turn off GSO/LRO so that
* user-space will not receive TSO frames.
*/
- if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6 | NETIF_F_UFO))
+ if (feature_mask & (NETIF_F_TSO | NETIF_F_TSO6))
features |= RX_OFFLOADS;
else
features &= ~RX_OFFLOADS;
@@ -1064,7 +1061,7 @@ static long macvtap_ioctl(struct file *file, unsigned int cmd,
case TUNSETOFFLOAD:
/* let the user check for future flags */
if (arg & ~(TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6 |
- TUN_F_TSO_ECN | TUN_F_UFO))
+ TUN_F_TSO_ECN))
return -EINVAL;
rtnl_lock();
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index 186ce54..e8b949a 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -174,7 +174,7 @@ struct tun_struct {
struct net_device *dev;
netdev_features_t set_features;
#define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
- NETIF_F_TSO6|NETIF_F_UFO)
+ NETIF_F_TSO6)
int vnet_hdr_sz;
int sndbuf;
@@ -1149,8 +1149,17 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ {
+ static bool warned;
+ if (!warned) {
+ warned = true;
+ netdev_warn(tun->dev,
+ "%s: using disabled UFO feature; please fix this program\n",
+ current->comm);
+ }
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
break;
+ }
default:
tun->dev->stats.rx_frame_errors++;
kfree_skb(skb);
@@ -1251,8 +1260,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (sinfo->gso_type & SKB_GSO_TCPV6)
gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
- else if (sinfo->gso_type & SKB_GSO_UDP)
- gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
else {
pr_err("unexpected GSO type: "
"0x%x, gso_size %d, hdr_len %d\n",
@@ -1762,11 +1769,6 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
features |= NETIF_F_TSO6;
arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
}
-
- if (arg & TUN_F_UFO) {
- features |= NETIF_F_UFO;
- arg &= ~TUN_F_UFO;
- }
}
/* This gives the user a way to test for new features in future by
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d75256bd..e9e269d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -491,8 +491,16 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
break;
case VIRTIO_NET_HDR_GSO_UDP:
+ {
+ static bool warned;
+ if (!warned) {
+ warned = true;
+ netdev_warn(dev,
+ "host using disabled UFO feature; please fix it\n");
+ }
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
break;
+ }
case VIRTIO_NET_HDR_GSO_TCPV6:
skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
break;
@@ -881,8 +889,6 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
else if (skb_shinfo(skb)->gso_type & SKB_GSO_TCPV6)
hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
- else if (skb_shinfo(skb)->gso_type & SKB_GSO_UDP)
- hdr->hdr.gso_type = VIRTIO_NET_HDR_GSO_UDP;
else
BUG();
if (skb_shinfo(skb)->gso_type & SKB_GSO_TCP_ECN)
@@ -1705,7 +1711,7 @@ static int virtnet_probe(struct virtio_device *vdev)
dev->features |= NETIF_F_HW_CSUM|NETIF_F_SG|NETIF_F_FRAGLIST;
if (virtio_has_feature(vdev, VIRTIO_NET_F_GSO)) {
- dev->hw_features |= NETIF_F_TSO | NETIF_F_UFO
+ dev->hw_features |= NETIF_F_TSO
| NETIF_F_TSO_ECN | NETIF_F_TSO6;
}
/* Individual feature bits: what can host handle? */
@@ -1715,11 +1721,9 @@ static int virtnet_probe(struct virtio_device *vdev)
dev->hw_features |= NETIF_F_TSO6;
if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_ECN))
dev->hw_features |= NETIF_F_TSO_ECN;
- if (virtio_has_feature(vdev, VIRTIO_NET_F_HOST_UFO))
- dev->hw_features |= NETIF_F_UFO;
if (gso)
- dev->features |= dev->hw_features & (NETIF_F_ALL_TSO|NETIF_F_UFO);
+ dev->features |= dev->hw_features & NETIF_F_ALL_TSO;
/* (!csum && gso) case will be fixed by register_netdev() */
}
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_CSUM))
@@ -1757,8 +1761,7 @@ static int virtnet_probe(struct virtio_device *vdev)
/* If we can receive ANY GSO packets, we must allocate large ones. */
if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO6) ||
- virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN) ||
- virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_UFO))
+ virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_ECN))
vi->big_packets = true;
if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
@@ -1952,9 +1955,9 @@ static struct virtio_device_id id_table[] = {
static unsigned int features[] = {
VIRTIO_NET_F_CSUM, VIRTIO_NET_F_GUEST_CSUM,
VIRTIO_NET_F_GSO, VIRTIO_NET_F_MAC,
- VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_UFO, VIRTIO_NET_F_HOST_TSO6,
+ VIRTIO_NET_F_HOST_TSO4, VIRTIO_NET_F_HOST_TSO6,
VIRTIO_NET_F_HOST_ECN, VIRTIO_NET_F_GUEST_TSO4, VIRTIO_NET_F_GUEST_TSO6,
- VIRTIO_NET_F_GUEST_ECN, VIRTIO_NET_F_GUEST_UFO,
+ VIRTIO_NET_F_GUEST_ECN,
VIRTIO_NET_F_MRG_RXBUF, VIRTIO_NET_F_STATUS, VIRTIO_NET_F_CTRL_VQ,
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN,
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ,
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* [PATCH net 2/2] drivers/net,ipv6: Select IPv6 fragment idents for virtio UFO packets
From: Ben Hutchings @ 2014-10-27 4:23 UTC (permalink / raw)
To: netdev; +Cc: Hannes Frederic Sowa, virtualization
In-Reply-To: <1414383425.16849.18.camel@decadent.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 4671 bytes --]
UFO is now disabled on all drivers that work with virtio net headers,
but userland may try to send UFO/IPv6 packets anyway. Instead of
sending with ID=0, we should select identifiers on their behalf (as we
used to).
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
Fixes: 916e4cf46d02 ("ipv6: reuse ip6_frag_id from ip6_ufo_append_data")
---
drivers/net/macvtap.c | 3 +++
drivers/net/tun.c | 6 +++++-
include/net/ipv6.h | 2 ++
net/ipv6/output_core.c | 34 ++++++++++++++++++++++++++++++++++
4 files changed, 44 insertions(+), 1 deletion(-)
diff --git a/drivers/net/macvtap.c b/drivers/net/macvtap.c
index 2aeaa61..6f226de 100644
--- a/drivers/net/macvtap.c
+++ b/drivers/net/macvtap.c
@@ -16,6 +16,7 @@
#include <linux/idr.h>
#include <linux/fs.h>
+#include <net/ipv6.h>
#include <net/net_namespace.h>
#include <net/rtnetlink.h>
#include <net/sock.h>
@@ -572,6 +573,8 @@ static int macvtap_skb_from_vnet_hdr(struct sk_buff *skb,
pr_warn_once("macvtap: %s: using disabled UFO feature; please fix this program\n",
current->comm);
gso_type = SKB_GSO_UDP;
+ if (skb->protocol == htons(ETH_P_IPV6))
+ ipv6_proxy_select_ident(skb);
break;
default:
return -EINVAL;
diff --git a/drivers/net/tun.c b/drivers/net/tun.c
index e8b949a..f8356b0 100644
--- a/drivers/net/tun.c
+++ b/drivers/net/tun.c
@@ -65,6 +65,7 @@
#include <linux/nsproxy.h>
#include <linux/virtio_net.h>
#include <linux/rcupdate.h>
+#include <net/ipv6.h>
#include <net/net_namespace.h>
#include <net/netns/generic.h>
#include <net/rtnetlink.h>
@@ -1139,6 +1140,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
break;
}
+ skb_reset_network_header(skb);
+
if (gso.gso_type != VIRTIO_NET_HDR_GSO_NONE) {
pr_debug("GSO!\n");
switch (gso.gso_type & ~VIRTIO_NET_HDR_GSO_ECN) {
@@ -1158,6 +1161,8 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
current->comm);
}
skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
+ if (skb->protocol == htons(ETH_P_IPV6))
+ ipv6_proxy_select_ident(skb);
break;
}
default:
@@ -1188,7 +1193,6 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
}
- skb_reset_network_header(skb);
skb_probe_transport_header(skb, 0);
rxhash = skb_get_hash(skb);
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 97f4720..4292929 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -671,6 +671,8 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add
return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr));
}
+void ipv6_proxy_select_ident(struct sk_buff *skb);
+
int ip6_dst_hoplimit(struct dst_entry *dst);
static inline int ip6_sk_dst_hoplimit(struct ipv6_pinfo *np, struct flowi6 *fl6,
diff --git a/net/ipv6/output_core.c b/net/ipv6/output_core.c
index fc24c39..97f41a3 100644
--- a/net/ipv6/output_core.c
+++ b/net/ipv6/output_core.c
@@ -3,11 +3,45 @@
* not configured or static. These functions are needed by GSO/GRO implementation.
*/
#include <linux/export.h>
+#include <net/ip.h>
#include <net/ipv6.h>
#include <net/ip6_fib.h>
#include <net/addrconf.h>
#include <net/secure_seq.h>
+/* This function exists only for tap drivers that must support broken
+ * clients requesting UFO without specifying an IPv6 fragment ID.
+ *
+ * This is similar to ipv6_select_ident() but we use an independent hash
+ * seed to limit information leakage.
+ *
+ * The network header must be set before calling this.
+ */
+void ipv6_proxy_select_ident(struct sk_buff *skb)
+{
+ static u32 ip6_proxy_idents_hashrnd __read_mostly;
+ struct in6_addr buf[2];
+ struct in6_addr *addrs;
+ u32 hash, id;
+
+ addrs = skb_header_pointer(skb,
+ skb_network_offset(skb) +
+ offsetof(struct ipv6hdr, saddr),
+ sizeof(buf), buf);
+ if (!addrs)
+ return;
+
+ net_get_random_once(&ip6_proxy_idents_hashrnd,
+ sizeof(ip6_proxy_idents_hashrnd));
+
+ hash = __ipv6_addr_jhash(&addrs[1], ip6_proxy_idents_hashrnd);
+ hash = __ipv6_addr_jhash(&addrs[0], hash);
+
+ id = ip_idents_reserve(hash, 1);
+ skb_shinfo(skb)->ip6_frag_id = htonl(id);
+}
+EXPORT_SYMBOL_GPL(ipv6_proxy_select_ident);
+
int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr)
{
u16 offset = sizeof(struct ipv6hdr);
--
Ben Hutchings
Theory and practice are closer in theory than in practice.
- John Levine, moderator of comp.compilers
[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH net 1/2] drivers/net: Disable UFO through virtio
From: Varka Bhadram @ 2014-10-27 4:45 UTC (permalink / raw)
To: Ben Hutchings, netdev; +Cc: Hannes Frederic Sowa, virtualization
In-Reply-To: <1414383748.16849.21.camel@decadent.org.uk>
On 10/27/2014 09:52 AM, Ben Hutchings wrote:
> IPv6 does not allow fragmentation by routers, so there is no
> fragmentation ID in the fixed header. UFO for IPv6 requires the ID to
> be passed separately, but there is no provision for this in the virtio
> net protocol.
>
> Until recently our software implementation of UFO/IPv6 generated a new
> ID, but this was a bug. Now we will use ID=0 for any UFO/IPv6 packet
> passed through a tap, which is even worse.
>
> Unfortunately there is no distinction between UFO/IPv4 and v6
> features, so disable UFO on taps and virtio_net completely until we
> have a proper solution.
>
> We cannot depend on VM managers respecting the tap feature flags, so
> keep accepting UFO packets but log a warning the first time we do
> this.
(...)
> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> index 186ce54..e8b949a 100644
> --- a/drivers/net/tun.c
> +++ b/drivers/net/tun.c
> @@ -174,7 +174,7 @@ struct tun_struct {
> struct net_device *dev;
> netdev_features_t set_features;
> #define TUN_USER_FEATURES (NETIF_F_HW_CSUM|NETIF_F_TSO_ECN|NETIF_F_TSO| \
> - NETIF_F_TSO6|NETIF_F_UFO)
> + NETIF_F_TSO6)
>
> int vnet_hdr_sz;
> int sndbuf;
> @@ -1149,8 +1149,17 @@ static ssize_t tun_get_user(struct tun_struct *tun, struct tun_file *tfile,
> skb_shinfo(skb)->gso_type = SKB_GSO_TCPV6;
> break;
> case VIRTIO_NET_HDR_GSO_UDP:
> + {
> + static bool warned;
one line space after declaration..
> + if (!warned) {
> + warned = true;
> + netdev_warn(tun->dev,
> + "%s: using disabled UFO feature; please fix this program\n",
> + current->comm);
> + }
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> break;
> + }
> default:
> tun->dev->stats.rx_frame_errors++;
> kfree_skb(skb);
> @@ -1251,8 +1260,6 @@ static ssize_t tun_put_user(struct tun_struct *tun,
> gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> else if (sinfo->gso_type & SKB_GSO_TCPV6)
> gso.gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> - else if (sinfo->gso_type & SKB_GSO_UDP)
> - gso.gso_type = VIRTIO_NET_HDR_GSO_UDP;
> else {
> pr_err("unexpected GSO type: "
> "0x%x, gso_size %d, hdr_len %d\n",
> @@ -1762,11 +1769,6 @@ static int set_offload(struct tun_struct *tun, unsigned long arg)
> features |= NETIF_F_TSO6;
> arg &= ~(TUN_F_TSO4|TUN_F_TSO6);
> }
> -
> - if (arg & TUN_F_UFO) {
> - features |= NETIF_F_UFO;
> - arg &= ~TUN_F_UFO;
> - }
> }
>
> /* This gives the user a way to test for new features in future by
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d75256bd..e9e269d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -491,8 +491,16 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> skb_shinfo(skb)->gso_type = SKB_GSO_TCPV4;
> break;
> case VIRTIO_NET_HDR_GSO_UDP:
> + {
> + static bool warned;
dto...
> + if (!warned) {
> + warned = true;
> + netdev_warn(dev,
> + "host using disabled UFO feature; please fix it\n");
> + }
> skb_shinfo(skb)->gso_type = SKB_GSO_UDP;
> break;
> + }
>
(...)
>
>
--
Thanks and Regards,
Varka Bhadram.
^ permalink raw reply
* [PATCH 00/11] treewide: mask then shift defects and style updates
From: Joe Perches @ 2014-10-27 5:24 UTC (permalink / raw)
To: linux-kernel, linux-nvme, dri-devel, ivtv-devel, linux-media,
patches, netdev, linux-usb, linux-parisc
Cc: alsa-devel, linux-wireless, linux-input
logical mask has lower precedence than shift but should be
done before the shift so parentheses are generally required.
And when masking with a fixed value after a shift, normal kernel
style has the shift on the left, then the shift on the right so
convert a few non-conforming uses.
Joe Perches (11):
block: nvme-scsi: Fix probable mask then right shift defects
radeon: evergreen: Fix probable mask then right shift defects
aiptek: Fix probable mask then right shift defects
dvb-net: Fix probable mask then right shift defects
cx25840/cx18: Use standard ordering of mask and shift
wm8350-core: Fix probable mask then right shift defect
iwlwifi: dvm: Fix probable mask then right shift defect
ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect
tty: ipwireless: Fix probable mask then right shift defects
hwa-hc: Fix probable mask then right shift defect
sound: ad1889: Fix probable mask then right shift defects
drivers/block/nvme-scsi.c | 12 ++++++------
drivers/gpu/drm/radeon/evergreen.c | 3 ++-
drivers/input/tablet/aiptek.c | 6 +++---
drivers/media/dvb-core/dvb_net.c | 4 +++-
drivers/media/i2c/cx25840/cx25840-core.c | 12 ++++++------
drivers/media/pci/cx18/cx18-av-core.c | 16 ++++++++--------
drivers/mfd/wm8350-core.c | 2 +-
drivers/net/wireless/iwlwifi/dvm/lib.c | 4 ++--
drivers/ssb/driver_chipcommon_pmu.c | 4 ++--
drivers/tty/ipwireless/hardware.c | 12 ++++++------
drivers/usb/host/hwa-hc.c | 2 +-
sound/pci/ad1889.c | 8 ++++----
12 files changed, 44 insertions(+), 41 deletions(-)
--
2.1.2
^ permalink raw reply
* [PATCH 07/11] iwlwifi: dvm: Fix probable mask then right shift defect
From: Joe Perches @ 2014-10-27 5:25 UTC (permalink / raw)
To: linux-kernel
Cc: Johannes Berg, Emmanuel Grumbach, Intel Linux Wireless,
John W. Linville, linux-wireless, netdev
In-Reply-To: <cover.1414387334.git.joe@perches.com>
Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.
Add parentheses around the mask.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/wireless/iwlwifi/dvm/lib.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/dvm/lib.c b/drivers/net/wireless/iwlwifi/dvm/lib.c
index 2191621..02e4ede 100644
--- a/drivers/net/wireless/iwlwifi/dvm/lib.c
+++ b/drivers/net/wireless/iwlwifi/dvm/lib.c
@@ -418,8 +418,8 @@ void iwlagn_bt_adjust_rssi_monitor(struct iwl_priv *priv, bool rssi_ena)
static bool iwlagn_bt_traffic_is_sco(struct iwl_bt_uart_msg *uart_msg)
{
- return BT_UART_MSG_FRAME3SCOESCO_MSK & uart_msg->frame3 >>
- BT_UART_MSG_FRAME3SCOESCO_POS;
+ return (BT_UART_MSG_FRAME3SCOESCO_MSK & uart_msg->frame3) >>
+ BT_UART_MSG_FRAME3SCOESCO_POS;
}
static void iwlagn_bt_traffic_change_work(struct work_struct *work)
--
2.1.2
^ permalink raw reply related
* [PATCH 08/11] ssb: driver_chip_comon_pmu: Fix probable mask then right shift defect
From: Joe Perches @ 2014-10-27 5:25 UTC (permalink / raw)
To: linux-kernel, Michael Buesch; +Cc: netdev
In-Reply-To: <cover.1414387334.git.joe@perches.com>
Precedence of & and >> is not the same and is not left to right.
shift has higher precedence and should be done after the mask.
Add parentheses around the mask.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/ssb/driver_chipcommon_pmu.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ssb/driver_chipcommon_pmu.c b/drivers/ssb/driver_chipcommon_pmu.c
index 1173a09..bc71583 100644
--- a/drivers/ssb/driver_chipcommon_pmu.c
+++ b/drivers/ssb/driver_chipcommon_pmu.c
@@ -621,8 +621,8 @@ static u32 ssb_pmu_get_alp_clock_clk0(struct ssb_chipcommon *cc)
u32 crystalfreq;
const struct pmu0_plltab_entry *e = NULL;
- crystalfreq = chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
- SSB_CHIPCO_PMU_CTL_XTALFREQ >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
+ crystalfreq = (chipco_read32(cc, SSB_CHIPCO_PMU_CTL) &
+ SSB_CHIPCO_PMU_CTL_XTALFREQ) >> SSB_CHIPCO_PMU_CTL_XTALFREQ_SHIFT;
e = pmu0_plltab_find_entry(crystalfreq);
BUG_ON(!e);
return e->freq * 1000;
--
2.1.2
^ permalink raw reply related
* RE: [PATCH 07/11] iwlwifi: dvm: Fix probable mask then right shift defect
From: Grumbach, Emmanuel @ 2014-10-27 6:14 UTC (permalink / raw)
To: Joe Perches, linux-kernel@vger.kernel.org
Cc: Berg, Johannes, Intel Linux Wireless, John W. Linville,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <83c3bfdd0c8f21fbbcbd9ebb5746a8fb04494fac.1414387334.git.joe@perches.com>
>
> Precedence of & and >> is not the same and is not left to right.
> shift has higher precedence and should be done after the mask.
>
> Add parentheses around the mask.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
Applied - thanks.
^ 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