* Re: [PATCH net] tcp: fix possible NULL dereference in tcp_vX_send_reset()
From: David Miller @ 2014-11-26 17:09 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev, dborkman, jasa.bartelj
In-Reply-To: <1416930004.29427.21.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Nov 2014 07:40:04 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> After commit ca777eff51f7 ("tcp: remove dst refcount false sharing for
> prequeue mode") we have to relax check against skb dst in
> tcp_v[46]_send_reset() if prequeue dropped the dst.
>
> If a socket is provided, a full lookup was done to find this socket,
> so the dst test can be skipped.
>
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88191
> Reported-by: Jaša Bartelj <jasa.bartelj@gmail.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Daniel Borkmann <dborkman@redhat.com>
> Fixes: ca777eff51f7 ("tcp: remove dst refcount false sharing for prequeue mode")
Applied, thanks Eric.
^ permalink raw reply
* Re: [patch net-next v3 04/17] net: introduce generic switch devices support
From: Jamal Hadi Salim @ 2014-11-26 17:09 UTC (permalink / raw)
To: Thomas Graf
Cc: Scott Feldman, Jiri Pirko, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville
In-Reply-To: <20141126160859.GB13786@casper.infradead.org>
On 11/26/14 11:08, Thomas Graf wrote:
> On 11/26/14 at 06:36am, Jamal Hadi Salim wrote:
>
>
> Jamal,
>
> What is irriating in this context is that you are pushing back on
> Jiri and others while referring to properitary and closed code which
> you are unwilling or unable to share. I don't see this as being
> passive aggressive, everybody is treated the same way in this regard.
>
WTF? I said i have hardware that is not a switch because it doesnt
do switching. This all started with the name being "switch" which
I objected to. You ask me to describe hardware and then you come
back and say I am using that to stop progress?
Where the hell did i push back on Jiri? Stop going around
telling people i do. I invest my time and effort reviewing code,
proposing ideas, posting etc calling meetings. Infact i initiated
this whole effort to begin with.
There is no point to responding to any of your other comments.
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next] pkt_sched: fq: increase max delay from 125 ms to one second
From: David Miller @ 2014-11-26 17:08 UTC (permalink / raw)
To: eric.dumazet; +Cc: yangyingliang, netdev, ncardwell
In-Reply-To: <1416934649.29427.34.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 25 Nov 2014 08:57:29 -0800
> From: Eric Dumazet <edumazet@google.com>
>
> FQ/pacing has a clamp of delay of 125 ms, to avoid some possible harm.
>
> It turns out this delay is too small to allow pacing low rates :
> Some ISP setup very aggressive policers as low as 16kbit.
>
> Now TCP stack has spurious rtx prevention, it seems safe to increase
> this fixed parameter, without adding a qdisc attribute.
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
Applied, thanks Eric.
^ permalink raw reply
* Re: [PATCH net] r8152: drop the tx packet with invalid length
From: David Miller @ 2014-11-26 17:06 UTC (permalink / raw)
To: eric.dumazet; +Cc: hayeswang, netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1417020748.29427.59.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 26 Nov 2014 08:52:28 -0800
> On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
>> Drop the tx packet which is more than the size of agg_buf_sz. When
>> creating a bridge with the device, we may get the tx packet with
>> TSO and the length is more than the gso_max_size which is set by
>> the driver through netif_set_gso_max_size(). Such packets couldn't
>> be transmitted and should be dropped directly.
>>
>> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
...
> Looks like a candidate for ndo_gso_check(), so that we do not drop, but
> instead segment from netif_needs_gso()/validate_xmit_skb()
You mean have the bridge implement the ndo_gso_check() method right?
^ permalink raw reply
* Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations
From: Thomas Graf @ 2014-11-26 17:06 UTC (permalink / raw)
To: John Fastabend; +Cc: Jiri Pirko, davem, stephen, netdev
In-Reply-To: <547606B3.3060808@gmail.com>
On 11/26/14 at 08:58am, John Fastabend wrote:
> On 11/26/2014 04:42 AM, Thomas Graf wrote:
> >Adds various missing length checks in the bridging code for Netlink
> >messages and corresponding attributes provided by user space.
> >
> >Thomas Graf (5):
> > bridge: Validate IFLA_BRIDGE_FLAGS attribute length
> > net: Validate IFLA_BRIDGE_MODE attribute length
> > net: Check for presence of IFLA_AF_SPEC
> > bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
> > bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
> >
> > drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
> > net/bridge/br_netlink.c | 1 +
> > net/core/rtnetlink.c | 23 ++++++++++++++++++-----
> > 4 files changed, 29 insertions(+), 5 deletions(-)
> >
>
> +Jiri
>
> Looks like a miss in bond_netlink also? Seems like writing
> a smatch or cocci check for this would be worthwhile.
Thanks, I'll take a look.
The cocci check is somewhat difficult as validation is often
centralized and decoupled from actual access to implement atomic
operations. I'll give it a try though.
^ permalink raw reply
* Re: [PATCH net-next v4] ipvlan: Initial check-in of the IPVLAN driver.
From: Mahesh Bandewar @ 2014-11-26 17:05 UTC (permalink / raw)
To: Toshiaki Makita
Cc: netdev, Eric Dumazet, Maciej Zenczykowski, Laurent Chavey,
Tim Hockin, David Miller, Brandon Philips, Pavel Emelianov
In-Reply-To: <54757631.8090604@lab.ntt.co.jp>
On Tue, Nov 25, 2014 at 10:41 PM, Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
> Hi Mahesh,
>
> I found that deleting the last ipvlan device triggers WARN_ON() in
> rtmsg_ifinfo().
> ipvlan_nl_fillinfo() seems to return -EINVAL in that case.
>
>> +static int ipvlan_nl_fillinfo(struct sk_buff *skb,
>> + const struct net_device *dev)
>> +{
>> + struct ipvl_dev *ipvlan = netdev_priv(dev);
>> + struct ipvl_port *port = ipvlan_port_get_rtnl(ipvlan->phy_dev);
>> + int ret = -EINVAL;
>> +
>> + if (!port)
>> + goto err;
>> +
>> + ret = -EMSGSIZE;
>> + if (nla_put_u16(skb, IFLA_IPVLAN_MODE, port->mode))
>> + goto err;
>> +
>> + return 0;
>> +
>> +err:
>> + return ret;
>> +}
>
> rollback_registered_many() calls rtmsg_ifinfo() after calling ndo_uninit().
> ndo_uninit() (ipvlan_uninit() -> ipvlan_port_destroy() ->
> netdev_rx_handler_unregister()) sets rx_handler_data into NULL.
> So, we cannot dereference "port" in ipvlan_nl_fillinfo().
>
Calling fillinfo() after calling uninit() seems pointless on any
device. But how are you hitting this case? Can you share the command
sequence with me?
Thanks,
--mahesh..
> Maybe "mode" should belong to struct ipvl_dev?
>
> Thanks,
> Toshiaki Makita
>
^ permalink raw reply
* Re: [PATCH net] net/mlx4_core: Limit count field to 24 bits in qp_alloc_res
From: David Miller @ 2014-11-26 17:05 UTC (permalink / raw)
To: ogerlitz; +Cc: netdev, matanb, amirv, jackm
In-Reply-To: <5475E19D.5000508@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Wed, 26 Nov 2014 16:20:13 +0200
> Currently, the upstream PF driver is wrongly NOT masking out these
> eight bits and as such, some VF drivers which are already setting
> them for optimized allocation are failing to allocate QPs as the
> count seen by the PF becomes way too large. The optimization is
> best effort anyway, and hence we can safely ignore their request.
Ok, thanks for explaining.
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH 0/5 net] bridge: Fix missing Netlink message validations
From: John Fastabend @ 2014-11-26 16:58 UTC (permalink / raw)
To: Thomas Graf, Jiri Pirko; +Cc: davem, stephen, netdev
In-Reply-To: <cover.1417005245.git.tgraf@suug.ch>
On 11/26/2014 04:42 AM, Thomas Graf wrote:
> Adds various missing length checks in the bridging code for Netlink
> messages and corresponding attributes provided by user space.
>
> Thomas Graf (5):
> bridge: Validate IFLA_BRIDGE_FLAGS attribute length
> net: Validate IFLA_BRIDGE_MODE attribute length
> net: Check for presence of IFLA_AF_SPEC
> bridge: Add missing policy entry for IFLA_BRPORT_FAST_LEAVE
> bridge: Sanitize IFLA_EXT_MASK for AF_BRIDGE:RTM_GETLINK
>
> drivers/net/ethernet/emulex/benet/be_main.c | 5 +++++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 5 +++++
> net/bridge/br_netlink.c | 1 +
> net/core/rtnetlink.c | 23 ++++++++++++++++++-----
> 4 files changed, 29 insertions(+), 5 deletions(-)
>
+Jiri
Looks like a miss in bond_netlink also? Seems like writing
a smatch or cocci check for this would be worthwhile.
>
> diff --git a/drivers/net/bonding/bond_netlink.c b/drivers/net/bonding/bond_netlink.c
> index 3e6eebd..7b11243 100644
> --- a/drivers/net/bonding/bond_netlink.c
> +++ b/drivers/net/bonding/bond_netlink.c
> @@ -225,7 +225,12 @@ static int bond_changelink(struct net_device *bond_dev,
>
> bond_option_arp_ip_targets_clear(bond);
> nla_for_each_nested(attr, data[IFLA_BOND_ARP_IP_TARGET], rem) {
> - __be32 target = nla_get_be32(attr);
> + __be32 target;
> +
> + if (nla_len(attr) < sizeof(target))
> + return -EINVAL;
> +
> + target = nla_get_be32(attr);
>
> bond_opt_initval(&newval, (__force u64)target);
> err = __bond_opt_set(bond, BOND_OPT_ARP_TARGETS,
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Joe Perches @ 2014-11-26 16:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAADnVQK8mn4ZHwbhBArosEw3Bmsc5vJfOzAeRE3R_1S7dbNfkg@mail.gmail.com>
On Wed, 2014-11-26 at 08:42 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 26, 2014 at 1:18 AM, Quentin Lambert
> <lambert.quentin@gmail.com> wrote:
> > Remove if then else statements preceding
> > boolean return.
[]
> > diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
[]
> > @@ -135,11 +135,9 @@ static const int reg2hex[] = {
> > */
> > static inline bool is_ereg(u32 reg)
> > {
> > - if (reg == BPF_REG_5 || reg == AUX_REG ||
> > - (reg >= BPF_REG_7 && reg <= BPF_REG_9))
> > - return true;
> > - else
> > - return false;
> > + return (reg == BPF_REG_5 ||
> > + reg == AUX_REG ||
> > + (reg >= BPF_REG_7 && reg <= BPF_REG_9));
>
> please remove extra () around the whole expression, and
> align in properly, and
> don't move reg==AUX_REG check to a different line.
> Subject is not warranted. I don't think it's a simplification.
It's not really a simplification,
gcc should emit the same object code.
> imo existing code is fine and I don't think the time spent
> reviewing such changes is worth it when there is no
> improvement in readability.
Is there any value in reordering these tests for frequency
or maybe using | instead of || to avoid multiple jumps?
^ permalink raw reply
* Re: [PATCH net] r8152: drop the tx packet with invalid length
From: Eric Dumazet @ 2014-11-26 16:52 UTC (permalink / raw)
To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <1394712342-15778-104-Taiwan-albertk@realtek.com>
On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
> Drop the tx packet which is more than the size of agg_buf_sz. When
> creating a bridge with the device, we may get the tx packet with
> TSO and the length is more than the gso_max_size which is set by
> the driver through netif_set_gso_max_size(). Such packets couldn't
> be transmitted and should be dropped directly.
>
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
> drivers/net/usb/r8152.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index c6554c7..ebdaff7 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -1897,6 +1897,15 @@ static netdev_tx_t rtl8152_start_xmit(struct sk_buff *skb,
> {
> struct r8152 *tp = netdev_priv(netdev);
>
> + if ((skb->len + sizeof(struct tx_desc)) > agg_buf_sz) {
> + struct net_device_stats *stats = &netdev->stats;
> +
> + dev_kfree_skb_any(skb);
> + stats->tx_dropped++;
> + WARN_ON_ONCE(1);
> + return NETDEV_TX_OK;
> + }
> +
> skb_tx_timestamp(skb);
>
> skb_queue_tail(&tp->tx_queue, skb);
Looks like a candidate for ndo_gso_check(), so that we do not drop, but
instead segment from netif_needs_gso()/validate_xmit_skb()
^ permalink raw reply
* Re: [PATCH 2/5] net: Validate IFLA_BRIDGE_MODE attribute length
From: John Fastabend @ 2014-11-26 16:42 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, stephen, netdev, Ajit Khaparde, John Fastabend
In-Reply-To: <4a88a0350064b5c2ec4e2adcef5afdfcab3e45dd.1417005245.git.tgraf@suug.ch>
On 11/26/2014 04:42 AM, Thomas Graf wrote:
> Payload is currently accessed blindly and may exceed valid message
> boundaries.
>
> Fixes: a77dcb8c8 ("be2net: set and query VEB/VEPA mode of the PF interface")
> Fixes: 815cccbf1 ("ixgbe: add setlink, getlink support to ixgbe and ixgbevf")
> Cc: Ajit Khaparde <ajit.khaparde@emulex.com>
> Cc: John Fastabend <john.r.fastabend@intel.com>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
> ---
> drivers/net/ethernet/emulex/benet/be_main.c | 3 +++
> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 3 +++
> 2 files changed, 6 insertions(+)
>
Thanks Thomas.
Acked-by: John Fastabend <john.r.fastabend@intel.com>
--
John Fastabend Intel Corporation
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Alexei Starovoitov @ 2014-11-26 16:42 UTC (permalink / raw)
To: Quentin Lambert
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, Nov 26, 2014 at 1:18 AM, Quentin Lambert
<lambert.quentin@gmail.com> wrote:
> Remove if then else statements preceding
> boolean return. Occurences were found using
> Coccinelle.
>
> The semantic patch used was:
>
> @@
> expression expr;
> @@
>
>
> - if ( expr )
> - return true;
> - else
> - return false;
> + return expr;
>
> Signed-off-by: Quentin Lambert <lambert.quentin@gmail.com>
>
> ---
> arch/x86/net/bpf_jit_comp.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 3f62734..1542f39 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -135,11 +135,9 @@ static const int reg2hex[] = {
> */
> static inline bool is_ereg(u32 reg)
> {
> - if (reg == BPF_REG_5 || reg == AUX_REG ||
> - (reg >= BPF_REG_7 && reg <= BPF_REG_9))
> - return true;
> - else
> - return false;
> + return (reg == BPF_REG_5 ||
> + reg == AUX_REG ||
> + (reg >= BPF_REG_7 && reg <= BPF_REG_9));
please remove extra () around the whole expression, and
align in properly, and
don't move reg==AUX_REG check to a different line.
Subject is not warranted. I don't think it's a simplification.
imo existing code is fine and I don't think the time spent
reviewing such changes is worth it when there is no
improvement in readability.
^ permalink raw reply
* Re: [patch net-next v3 04/17] net: introduce generic switch devices support
From: Thomas Graf @ 2014-11-26 16:08 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Scott Feldman, Jiri Pirko, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville
In-Reply-To: <5475BB53.3070200@mojatatu.com>
On 11/26/14 at 06:36am, Jamal Hadi Salim wrote:
> On 11/25/14 23:18, Scott Feldman wrote:
> >On Tue, Nov 25, 2014 at 5:33 PM, Jamal Hadi Salim <jhs@mojatatu.com> wrote:
>
> >
> >You have a pointer to the kernel driver for that HW?
>
> I wasnt sure if that was a passive aggressive move there to
> question what i am claiming?(Only Canadians are allowed to be
> passive aggressive Scott). To answer your question, no
> code currently littered with vendor SDK unfortunately (as you
> would know!).
> But hopefully if we get these changes in correctly it would
> not be hard to show the driver working fully in the kernel.
> There are definetely a few other pieces of hardware that are
> making me come back here and invest time and effort in these
> long discussions.
>
> >Can you show how
> >you're using Linux tc netlink msg in kernel to program HW? I'd like
> >to see the in-kernel API.
> >
>
> Lets do the L2/port thing first. But yes, I am using Linux tc in
> kernel.
Jamal,
What is irriating in this context is that you are pushing back on
Jiri and others while referring to properitary and closed code which
you are unwilling or unable to share. I don't see this as being
passive aggressive, everybody is treated the same way in this regard.
It is exactly the point of this API and related discussions to
decouple the control plane (tc) from any vendor specifics while
allowing them to innovate, compete, and solve different use cases.
I think it's absolutely the right thing to write the API against
code that is public, which in this case is rocker and the existing
in-kernel NIC drivers.
^ permalink raw reply
* Re: [PATCH v5 2/4] arch: Add lightweight memory barriers dma_rmb() and dma_wmb()
From: Will Deacon @ 2014-11-26 16:04 UTC (permalink / raw)
To: Alexander Duyck
Cc: linux-arch@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, mathieu.desnoyers@polymtl.ca,
peterz@infradead.org, benh@kernel.crashing.org,
heiko.carstens@de.ibm.com, mingo@kernel.org, mikey@neuling.org,
linux@arm.linux.org.uk, donald.c.skidmore@intel.com,
matthew.vick@intel.com, geert@linux-m68k.org,
jeffrey.t.kirsher@intel.com, romieu@fr.zoreil.com,
paulmck@linux.vnet.ibm.com,
"nic_swsd@realtek.com" <nic_
In-Reply-To: <5474ADB4.5070200@redhat.com>
On Tue, Nov 25, 2014 at 04:26:28PM +0000, Alexander Duyck wrote:
> On 11/25/2014 06:01 AM, Will Deacon wrote:
> > If we ever see platforms using Linux/dma_alloc_coherent with devices
> > mastering from a different outer-shareable domain that the one containing
> > the CPUs, then we'll need to revisit this.
>
> Would we just need a system wide memory barrier in that case instead of
> an outer shareable memory barrier, or would we need to look as something
> like a sync barrier?
I think dmb(sy) would do the trick, but let's cross that bridge if/when we
have to.
Will
^ permalink raw reply
* Re: [PATCH RFC net-next] net: Add GRO support for GRE tunneling of TEB packets
From: Tom Herbert @ 2014-11-26 15:44 UTC (permalink / raw)
To: Or Gerlitz
Cc: David S. Miller, Linux Netdev List, Eric Dumazet, H.K. Jerry Chu
In-Reply-To: <1417014504-5929-1-git-send-email-ogerlitz@mellanox.com>
On Wed, Nov 26, 2014 at 7:08 AM, Or Gerlitz <ogerlitz@mellanox.com> wrote:
> Add the missing parts in the gre gro handlers when the inner protocol
> is ETH_P_TEB which is the case for OVS based GRE tunneling.
>
> Cc: H.K. Jerry Chu <hkchu@google.com>
> Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
> ---
> net/ipv4/gre_offload.c | 57 +++++++++++++++++++++++++++++++++++++----------
> 1 files changed, 45 insertions(+), 12 deletions(-)
>
> diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
> index bb5947b..06ae197 100644
> --- a/net/ipv4/gre_offload.c
> +++ b/net/ipv4/gre_offload.c
> @@ -14,6 +14,7 @@
> #include <linux/init.h>
> #include <net/protocol.h>
> #include <net/gre.h>
> +#include <linux/etherdevice.h>
>
> static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
> netdev_features_t features)
> @@ -121,8 +122,9 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
> struct sk_buff **pp = NULL;
> struct sk_buff *p;
> const struct gre_base_hdr *greh;
> + struct ethhdr *eh = NULL;
> unsigned int hlen, grehlen;
> - unsigned int off;
> + unsigned int off, off_eth = 0;
> int flush = 1;
> struct packet_offload *ptype;
> __be16 type;
> @@ -147,11 +149,6 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
>
> type = greh->protocol;
>
> - rcu_read_lock();
> - ptype = gro_find_receive_by_type(type);
> - if (ptype == NULL)
> - goto out_unlock;
> -
> grehlen = GRE_HEADER_SECTION;
>
> if (greh->flags & GRE_KEY)
> @@ -164,22 +161,45 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
> if (skb_gro_header_hard(skb, hlen)) {
> greh = skb_gro_header_slow(skb, hlen, off);
> if (unlikely(!greh))
> - goto out_unlock;
> + goto out;
> }
>
> /* Don't bother verifying checksum if we're going to flush anyway. */
> if ((greh->flags & GRE_CSUM) && !NAPI_GRO_CB(skb)->flush) {
> if (skb_gro_checksum_simple_validate(skb))
> - goto out_unlock;
> + goto out;
>
> skb_gro_checksum_try_convert(skb, IPPROTO_GRE, 0,
> null_compute_pseudo);
> }
>
> + skb_gro_pull(skb, grehlen);
> +
> + /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
> + skb_gro_postpull_rcsum(skb, greh, grehlen);
> +
> + if (type == ntohs(ETH_P_TEB)) {
> + off_eth = skb_gro_offset(skb);
> + hlen = off_eth + sizeof(*eh);
> + eh = skb_gro_header_fast(skb, off_eth);
> + if (skb_gro_header_hard(skb, hlen)) {
> + eh = skb_gro_header_slow(skb, hlen, off_eth);
> + if (unlikely(!eh))
> + goto out;
> + }
> + type = eh->h_proto;
I don't think this is the right approach. It would probably be better
to a add a gro_receive handler for ETH_P_TEB and then you wouldn't
need to modify GRE path with special case code. That would also be
applicable in geneve.
> + }
> +
> + rcu_read_lock();
> + ptype = gro_find_receive_by_type(type);
> + if (ptype == NULL)
> + goto out_unlock;
> +
> flush = 0;
>
> for (p = *head; p; p = p->next) {
> const struct gre_base_hdr *greh2;
> + const struct ethhdr *eh2;
>
> if (!NAPI_GRO_CB(p)->same_flow)
> continue;
> @@ -205,13 +225,19 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
> NAPI_GRO_CB(p)->same_flow = 0;
> continue;
> }
> +
> + eh2 = (struct ethhdr *)(p->data + off_eth);
> + if (eh && compare_ether_header(eh, eh2)) {
> + NAPI_GRO_CB(p)->same_flow = 0;
> + continue;
> + }
> }
> }
>
> - skb_gro_pull(skb, grehlen);
> -
> - /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
> - skb_gro_postpull_rcsum(skb, greh, grehlen);
> + if (eh) {
> + skb_gro_pull(skb, sizeof(*eh)); /* pull inner eth header */
> + skb_gro_postpull_rcsum(skb, eh, sizeof(*eh));
> + }
>
> pp = ptype->callbacks.gro_receive(head, skb);
>
> @@ -229,6 +255,7 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
> struct packet_offload *ptype;
> unsigned int grehlen = sizeof(*greh);
> int err = -ENOENT;
> + struct ethhdr *eh;
> __be16 type;
>
> skb->encapsulation = 1;
> @@ -241,6 +268,12 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
> if (greh->flags & GRE_CSUM)
> grehlen += GRE_HEADER_SECTION;
>
> + if (type == ntohs(ETH_P_TEB)) {
> + eh = (struct ethhdr *)(skb->data + nhoff + grehlen);
> + type = eh->h_proto;
> + grehlen += sizeof(*eh);
> + }
> +
> rcu_read_lock();
> ptype = gro_find_complete_by_type(type);
> if (ptype != NULL)
> --
> 1.7.1
>
^ permalink raw reply
* [PATCH RFC net-next] net: Add GRO support for GRE tunneling of TEB packets
From: Or Gerlitz @ 2014-11-26 15:08 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, therbert, edumazet, Or Gerlitz, H.K. Jerry Chu
Add the missing parts in the gre gro handlers when the inner protocol
is ETH_P_TEB which is the case for OVS based GRE tunneling.
Cc: H.K. Jerry Chu <hkchu@google.com>
Signed-off-by: Or Gerlitz <ogerlitz@mellanox.com>
---
net/ipv4/gre_offload.c | 57 +++++++++++++++++++++++++++++++++++++----------
1 files changed, 45 insertions(+), 12 deletions(-)
diff --git a/net/ipv4/gre_offload.c b/net/ipv4/gre_offload.c
index bb5947b..06ae197 100644
--- a/net/ipv4/gre_offload.c
+++ b/net/ipv4/gre_offload.c
@@ -14,6 +14,7 @@
#include <linux/init.h>
#include <net/protocol.h>
#include <net/gre.h>
+#include <linux/etherdevice.h>
static struct sk_buff *gre_gso_segment(struct sk_buff *skb,
netdev_features_t features)
@@ -121,8 +122,9 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
struct sk_buff **pp = NULL;
struct sk_buff *p;
const struct gre_base_hdr *greh;
+ struct ethhdr *eh = NULL;
unsigned int hlen, grehlen;
- unsigned int off;
+ unsigned int off, off_eth = 0;
int flush = 1;
struct packet_offload *ptype;
__be16 type;
@@ -147,11 +149,6 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
type = greh->protocol;
- rcu_read_lock();
- ptype = gro_find_receive_by_type(type);
- if (ptype == NULL)
- goto out_unlock;
-
grehlen = GRE_HEADER_SECTION;
if (greh->flags & GRE_KEY)
@@ -164,22 +161,45 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
if (skb_gro_header_hard(skb, hlen)) {
greh = skb_gro_header_slow(skb, hlen, off);
if (unlikely(!greh))
- goto out_unlock;
+ goto out;
}
/* Don't bother verifying checksum if we're going to flush anyway. */
if ((greh->flags & GRE_CSUM) && !NAPI_GRO_CB(skb)->flush) {
if (skb_gro_checksum_simple_validate(skb))
- goto out_unlock;
+ goto out;
skb_gro_checksum_try_convert(skb, IPPROTO_GRE, 0,
null_compute_pseudo);
}
+ skb_gro_pull(skb, grehlen);
+
+ /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
+ skb_gro_postpull_rcsum(skb, greh, grehlen);
+
+ if (type == ntohs(ETH_P_TEB)) {
+ off_eth = skb_gro_offset(skb);
+ hlen = off_eth + sizeof(*eh);
+ eh = skb_gro_header_fast(skb, off_eth);
+ if (skb_gro_header_hard(skb, hlen)) {
+ eh = skb_gro_header_slow(skb, hlen, off_eth);
+ if (unlikely(!eh))
+ goto out;
+ }
+ type = eh->h_proto;
+ }
+
+ rcu_read_lock();
+ ptype = gro_find_receive_by_type(type);
+ if (ptype == NULL)
+ goto out_unlock;
+
flush = 0;
for (p = *head; p; p = p->next) {
const struct gre_base_hdr *greh2;
+ const struct ethhdr *eh2;
if (!NAPI_GRO_CB(p)->same_flow)
continue;
@@ -205,13 +225,19 @@ static struct sk_buff **gre_gro_receive(struct sk_buff **head,
NAPI_GRO_CB(p)->same_flow = 0;
continue;
}
+
+ eh2 = (struct ethhdr *)(p->data + off_eth);
+ if (eh && compare_ether_header(eh, eh2)) {
+ NAPI_GRO_CB(p)->same_flow = 0;
+ continue;
+ }
}
}
- skb_gro_pull(skb, grehlen);
-
- /* Adjusted NAPI_GRO_CB(skb)->csum after skb_gro_pull()*/
- skb_gro_postpull_rcsum(skb, greh, grehlen);
+ if (eh) {
+ skb_gro_pull(skb, sizeof(*eh)); /* pull inner eth header */
+ skb_gro_postpull_rcsum(skb, eh, sizeof(*eh));
+ }
pp = ptype->callbacks.gro_receive(head, skb);
@@ -229,6 +255,7 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
struct packet_offload *ptype;
unsigned int grehlen = sizeof(*greh);
int err = -ENOENT;
+ struct ethhdr *eh;
__be16 type;
skb->encapsulation = 1;
@@ -241,6 +268,12 @@ static int gre_gro_complete(struct sk_buff *skb, int nhoff)
if (greh->flags & GRE_CSUM)
grehlen += GRE_HEADER_SECTION;
+ if (type == ntohs(ETH_P_TEB)) {
+ eh = (struct ethhdr *)(skb->data + nhoff + grehlen);
+ type = eh->h_proto;
+ grehlen += sizeof(*eh);
+ }
+
rcu_read_lock();
ptype = gro_find_complete_by_type(type);
if (ptype != NULL)
--
1.7.1
^ permalink raw reply related
* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Michael S. Tsirkin @ 2014-11-26 15:01 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126155440.4f49b70f.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 03:54:40PM +0100, Cornelia Huck wrote:
> On Wed, 26 Nov 2014 16:44:00 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Nov 26, 2014 at 03:31:02PM +0100, Cornelia Huck wrote:
> > > On Tue, 25 Nov 2014 18:43:14 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > We use native endian-ness internally but never
> > > > expose it to guest.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > drivers/vhost/net.c | 10 +++++-----
> > > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > index 8dae2f7..dce5c58 100644
> > > > --- a/drivers/vhost/net.c
> > > > +++ b/drivers/vhost/net.c
> > > > @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> > > > * status internally; used for zerocopy tx only.
> > > > */
> > > > /* Lower device DMA failed */
> > > > -#define VHOST_DMA_FAILED_LEN 3
> > > > +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> > > > /* Lower device DMA done */
> > > > -#define VHOST_DMA_DONE_LEN 2
> > > > +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> > > > /* Lower device DMA in progress */
> > > > -#define VHOST_DMA_IN_PROGRESS 1
> > > > +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> > > > /* Buffer unused */
> > > > -#define VHOST_DMA_CLEAR_LEN 0
> > > > +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
> > >
> > > I find these constants a bit confusing: What does __virtio32 mean
> > > without the context of a vq or device?
> > >
> > > >
> > > > -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> > > > +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
> > >
> > > And here you cast it to a plain u32 again.
> > >
> > > I looked at the final code, and you seem either to use the above
> > > constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
> > > convert the constants as well?
> >
> > I tried to explain it in the commit message.
> > It's a hack in vhost: it keeps virtio used structure in host
> > memory, but abuses length field for internal housekeeping.
> > This works because length in used ring for tx is always 0.
>
> Ah, ok. It might make sense to add this explanation to the patch :)
Absolutely.
^ permalink raw reply
* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Cornelia Huck @ 2014-11-26 14:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126144400.GA8086@redhat.com>
On Wed, 26 Nov 2014 16:44:00 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Nov 26, 2014 at 03:31:02PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:43:14 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > We use native endian-ness internally but never
> > > expose it to guest.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vhost/net.c | 10 +++++-----
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 8dae2f7..dce5c58 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> > > * status internally; used for zerocopy tx only.
> > > */
> > > /* Lower device DMA failed */
> > > -#define VHOST_DMA_FAILED_LEN 3
> > > +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> > > /* Lower device DMA done */
> > > -#define VHOST_DMA_DONE_LEN 2
> > > +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> > > /* Lower device DMA in progress */
> > > -#define VHOST_DMA_IN_PROGRESS 1
> > > +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> > > /* Buffer unused */
> > > -#define VHOST_DMA_CLEAR_LEN 0
> > > +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
> >
> > I find these constants a bit confusing: What does __virtio32 mean
> > without the context of a vq or device?
> >
> > >
> > > -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> > > +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
> >
> > And here you cast it to a plain u32 again.
> >
> > I looked at the final code, and you seem either to use the above
> > constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
> > convert the constants as well?
>
> I tried to explain it in the commit message.
> It's a hack in vhost: it keeps virtio used structure in host
> memory, but abuses length field for internal housekeeping.
> This works because length in used ring for tx is always 0.
Ah, ok. It might make sense to add this explanation to the patch :)
^ permalink raw reply
* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Michael S. Tsirkin @ 2014-11-26 14:44 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126153102.3f5b8fc9.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 03:31:02PM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 18:43:14 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > We use native endian-ness internally but never
> > expose it to guest.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/net.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 8dae2f7..dce5c58 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> > * status internally; used for zerocopy tx only.
> > */
> > /* Lower device DMA failed */
> > -#define VHOST_DMA_FAILED_LEN 3
> > +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> > /* Lower device DMA done */
> > -#define VHOST_DMA_DONE_LEN 2
> > +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> > /* Lower device DMA in progress */
> > -#define VHOST_DMA_IN_PROGRESS 1
> > +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> > /* Buffer unused */
> > -#define VHOST_DMA_CLEAR_LEN 0
> > +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
>
> I find these constants a bit confusing: What does __virtio32 mean
> without the context of a vq or device?
>
> >
> > -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> > +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
>
> And here you cast it to a plain u32 again.
>
> I looked at the final code, and you seem either to use the above
> constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
> convert the constants as well?
I tried to explain it in the commit message.
It's a hack in vhost: it keeps virtio used structure in host
memory, but abuses length field for internal housekeeping.
This works because length in used ring for tx is always 0.
> >
> > enum {
> > VHOST_NET_FEATURES = VHOST_FEATURES |
^ permalink raw reply
* Re: [PATCH net] net/mlx4_core: Limit count field to 24 bits in qp_alloc_res
From: Or Gerlitz @ 2014-11-26 14:20 UTC (permalink / raw)
To: David Miller; +Cc: netdev, matanb, amirv, jackm
In-Reply-To: <20141125.141620.1331667846639491375.davem@davemloft.net>
On 11/25/2014 9:16 PM, David Miller wrote:
> From: Or Gerlitz <ogerlitz@mellanox.com>
> Date: Tue, 25 Nov 2014 11:54:31 +0200
>
>> case RES_OP_RESERVE:
>> - count = get_param_l(&in_param);
>> + count = get_param_l(&in_param) & 0xffffff;
> I think if these high bits are set, you should be using the maximum
> value rather then truncating.
Dave,
To make it clear (maybe the change-log wasn't explaining it well
enough), the 32 bits in_param are
divided to 24 bits of count and 8 bits for optimized allocation scheme
which isn't yet supported
by the Linux PF driver.
Currently, the upstream PF driver is wrongly NOT masking out these eight
bits and as such, some
VF drivers which are already setting them for optimized allocation are
failing to allocate QPs as
the count seen by the PF becomes way too large. The optimization is
best effort anyway, and hence
we can safely ignore their request.
If we follow your suggestion and allocate to these VFs per the maximum
number, we will
hit their quota after they open one QP and their over-all bring up will
fail, so this will not help.
Or.
^ permalink raw reply
* Re: [PATCH v4 26/42] vhost/net: force len for TX to host endian
From: Cornelia Huck @ 2014-11-26 14:31 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <1416933600-21398-27-git-send-email-mst@redhat.com>
On Tue, 25 Nov 2014 18:43:14 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> We use native endian-ness internally but never
> expose it to guest.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/net.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 8dae2f7..dce5c58 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -48,15 +48,15 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
> * status internally; used for zerocopy tx only.
> */
> /* Lower device DMA failed */
> -#define VHOST_DMA_FAILED_LEN 3
> +#define VHOST_DMA_FAILED_LEN ((__force __virtio32)3)
> /* Lower device DMA done */
> -#define VHOST_DMA_DONE_LEN 2
> +#define VHOST_DMA_DONE_LEN ((__force __virtio32)2)
> /* Lower device DMA in progress */
> -#define VHOST_DMA_IN_PROGRESS 1
> +#define VHOST_DMA_IN_PROGRESS ((__force __virtio32)1)
> /* Buffer unused */
> -#define VHOST_DMA_CLEAR_LEN 0
> +#define VHOST_DMA_CLEAR_LEN ((__force __virtio32)0)
I find these constants a bit confusing: What does __virtio32 mean
without the context of a vq or device?
>
> -#define VHOST_DMA_IS_DONE(len) ((len) >= VHOST_DMA_DONE_LEN)
> +#define VHOST_DMA_IS_DONE(len) ((__force u32)(len) >= (__force u32)VHOST_DMA_DONE_LEN)
And here you cast it to a plain u32 again.
I looked at the final code, and you seem either to use the above
constants for .len or do a cpu_to_vhost32(). Wouldn't you need to
convert the constants as well?
>
> enum {
> VHOST_NET_FEATURES = VHOST_FEATURES |
^ permalink raw reply
* Re: [PATCH v4 25/42] vhost: add memory access wrappers
From: Michael S. Tsirkin @ 2014-11-26 14:24 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126151750.61a40fe4.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 03:17:50PM +0100, Cornelia Huck wrote:
> On Wed, 26 Nov 2014 16:05:39 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Nov 26, 2014 at 02:54:38PM +0100, Cornelia Huck wrote:
> > > On Tue, 25 Nov 2014 18:43:10 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > > > @@ -174,6 +174,37 @@ enum {
> > > >
> > > > static inline int vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > > > {
> > > > - return vq->acked_features & (1 << bit);
> > > > + return vq->acked_features & (1ULL << bit);
> > >
> > > Should this hunk go into patch 28?
> >
> > Well, this is needed here since 1 << 32 is not legal C.
> >
> > I can move it - this means patch 28 will have to move earlier
> > in series though.
>
> Yes, I think it makes sense to move patch 28 earlier.
Will do, thanks.
--
MST
^ permalink raw reply
* Re: [PATCH v4 25/42] vhost: add memory access wrappers
From: Cornelia Huck @ 2014-11-26 14:17 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126140539.GA5528@redhat.com>
On Wed, 26 Nov 2014 16:05:39 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Nov 26, 2014 at 02:54:38PM +0100, Cornelia Huck wrote:
> > On Tue, 25 Nov 2014 18:43:10 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > @@ -174,6 +174,37 @@ enum {
> > >
> > > static inline int vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > > {
> > > - return vq->acked_features & (1 << bit);
> > > + return vq->acked_features & (1ULL << bit);
> >
> > Should this hunk go into patch 28?
>
> Well, this is needed here since 1 << 32 is not legal C.
>
> I can move it - this means patch 28 will have to move earlier
> in series though.
Yes, I think it makes sense to move patch 28 earlier.
^ permalink raw reply
* [PATCH net-next] sky2: Fix crash inside sky2_rx_clean
From: Mirko Lindner @ 2014-11-26 14:13 UTC (permalink / raw)
To: davem, netdev@vger.kernel.org
If sky2->tx_le = pci_alloc_consistent() or sky2->tx_ring = kcalloc() in
sky2_alloc_buffers() fails, sky2->rx_ring = kcalloc() will never be called.
In this error case handling, sky2_rx_clean() is called from within
sky2_free_buffers().
In sky2_rx_clean() we find the following:
...
memset(sky2->rx_le, 0, RX_LE_BYTES);
...
This results in a memset using a NULL pointer and will crash the system.
Signed-off-by: Mirko Lindner <mlindner@marvell.com>
---
drivers/net/ethernet/marvell/sky2.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 53a1cc5..f8ab220 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -1361,7 +1361,9 @@ static void sky2_rx_clean(struct sky2_port *sky2)
{
unsigned i;
- memset(sky2->rx_le, 0, RX_LE_BYTES);
+ if (sky2->rx_le)
+ memset(sky2->rx_le, 0, RX_LE_BYTES);
+
for (i = 0; i < sky2->rx_pending; i++) {
struct rx_ring_info *re = sky2->rx_ring + i;
--
2.1.3
^ permalink raw reply related
* Re: [PATCH v4 25/42] vhost: add memory access wrappers
From: Michael S. Tsirkin @ 2014-11-26 14:05 UTC (permalink / raw)
To: Cornelia Huck
Cc: kvm, rusty, netdev, linux-kernel, virtualization, pbonzini,
David Miller
In-Reply-To: <20141126145438.4421c00d.cornelia.huck@de.ibm.com>
On Wed, Nov 26, 2014 at 02:54:38PM +0100, Cornelia Huck wrote:
> On Tue, 25 Nov 2014 18:43:10 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> "These wrappers are needed to handle virtio endianness conversions."
>
> ?
yes, it's same as virtio ones. I'll add this text, thanks.
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/vhost.h | 33 ++++++++++++++++++++++++++++++++-
> > 1 file changed, 32 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> > index 3eda654..b9032e8 100644
> > --- a/drivers/vhost/vhost.h
> > +++ b/drivers/vhost/vhost.h
> > @@ -174,6 +174,37 @@ enum {
> >
> > static inline int vhost_has_feature(struct vhost_virtqueue *vq, int bit)
> > {
> > - return vq->acked_features & (1 << bit);
> > + return vq->acked_features & (1ULL << bit);
>
> Should this hunk go into patch 28?
Well, this is needed here since 1 << 32 is not legal C.
I can move it - this means patch 28 will have to move earlier
in series though.
> > +}
^ 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