* Re: [PATCH net-next] net: sched: act_tunnel_key: fix metadata handling
From: Davide Caratti @ 2019-02-25 14:04 UTC (permalink / raw)
To: Vlad Buslov, netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, wenxu, roid
In-Reply-To: <20190225122122.8128-1-vladbu@mellanox.com>
On Mon, 2019-02-25 at 14:21 +0200, Vlad Buslov wrote:
> Tunnel key action params->tcft_enc_metadata is only set when action is
> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
> dereferenced during tunnel key init and release without verifying that
> action is if correct type, which causes NULL pointer dereference. Metadata
> tunnel dst_cache is also leaked on action overwrite.
>
> Fix metadata handling:
> - Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
hello Vlad,
thanks a lot for fixing this!
<...>
> @@ -384,10 +390,12 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>
> release_dst_cache:
> #ifdef CONFIG_DST_CACHE
> - dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> + if (metadata)
> + dst_cache_destroy(&metadata->u.tun_info.dst_cache);
> #endif
> release_tun_meta:
> - dst_release(&metadata->dst);
> + if (metadata)
> + dst_release(&metadata->dst);
on Linux 'net' tree we don't have commit 41411e2fd6b8 ("net/sched:
act_tunnel_key: Add dst_cache support"), but still the above two lines can
avoid a NULL dereference in tunnel_key_init() error path, in the following
case:
* create an action with tunnel "set", with success
* replace the previous rule rule with tunnel "unset", and have a failure
here (e.g. allocation of 'params_new').
At the cost of creating some conflicts during the merge, it would probably
be safer to split this commit into two parts, one targeting 'net' and one
targeting 'net-next', so that the first one can be proposed for stable
backports (and also I can rebase/retest my 'goto chain' series on top of
it :) )
WDYT?
--
davide
^ permalink raw reply
* Re: [PATCH net] sctp: get sctphdr by offset in sctp_compute_cksum
From: Neil Horman @ 2019-02-25 14:07 UTC (permalink / raw)
To: Xin Long; +Cc: LKML, network dev, linux-sctp, davem, Marcelo Ricardo Leitner
In-Reply-To: <CADvbK_cC4q1D5w1AHAVQjCTW4Pm6H+-ZOVg7xCp+eiOkkRJ8SQ@mail.gmail.com>
On Mon, Feb 25, 2019 at 09:20:44PM +0800, Xin Long wrote:
> On Mon, Feb 25, 2019 at 8:47 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> >
> > On Mon, Feb 25, 2019 at 07:25:37PM +0800, Xin Long wrote:
> > > sctp_hdr(skb) only works when skb->transport_header is set properly.
> > >
> > > But in the path of nf_conntrack_in: sctp_packet() -> sctp_error()
> > >
> > > skb->transport_header is not guaranteed to be right value for sctp.
> > > It will cause to fail to check the checksum for sctp packets.
> > >
> > > So fix it by using offset, which is always right in all places.
> > >
> > > Fixes: e6d8b64b34aa ("net: sctp: fix and consolidate SCTP checksumming code")
> > > Reported-by: Li Shuang <shuali@redhat.com>
> > > Signed-off-by: Xin Long <lucien.xin@gmail.com>
> > > ---
> > > include/net/sctp/checksum.h | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/sctp/checksum.h b/include/net/sctp/checksum.h
> > > index 32ee65a..1c6e6c0 100644
> > > --- a/include/net/sctp/checksum.h
> > > +++ b/include/net/sctp/checksum.h
> > > @@ -61,7 +61,7 @@ static inline __wsum sctp_csum_combine(__wsum csum, __wsum csum2,
> > > static inline __le32 sctp_compute_cksum(const struct sk_buff *skb,
> > > unsigned int offset)
> > > {
> > > - struct sctphdr *sh = sctp_hdr(skb);
> > > + struct sctphdr *sh = (struct sctphdr *)(skb->data + offset);
> > > const struct skb_checksum_ops ops = {
> > > .update = sctp_csum_update,
> > > .combine = sctp_csum_combine,
> > > --
> > > 2.1.0
> > >
> > >
> > Shouldn't you use skb_set_transport_header and skb_transport_header here?
> you mean:
> skb_set_transport_header(skb, offset);
> sh = sctp_hdr(skb);
> ?
>
> There's no place counting on here to set transport_header.
> It will be a kinda redundant job, yet skb is 'const'.
>
I'm not sure what you mean by "theres no place counting here". We have the
transport header offset, and you're doing the exact same computation that that
function does. It seems like we should use it in case the underlying
implementation changes.
I understand what you are saying regarding the use of a const variable there,
but perhaps thats an argument for removing the const storage classifier. Better
still, it would be good to figure out why all paths to this function don't
already set the transport header offset to begin with (addressing your redundant
comment)
Regards
Neil
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Dave Taht @ 2019-02-25 14:11 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Leslie Monis, netdev, dave.taht
In-Reply-To: <19575745-ad10-be2d-6259-5fe39fa71264@mojatatu.com>
Jamal Hadi Salim <jhs@mojatatu.com> writes:
> On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
>> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
>>> The current implementation of the PIE queuing discipline is
>>> according to the
>>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and
>>> the paper
>>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
>>> However, a lot of necessary modifications and enhancements have
>>> been proposed
>>> in RFC 8033, which have not yet been incorporated in the source
>>> code of Linux.
>>> This patch series helps in achieving the same.
>>>
>>> Performance tests carried out using Flent [https://flent.org/]
>>>
>>
>> +Cc the authors of PIE to double check these values.
>> Please respond (it is how open source works!) and N/ACK
>
> Great. Bouncing addresses.
> D. Taht - can you look at this?
Whose addresses are bouncing? The folk that did pie originally were
mostly contractors and long ago moved on to other things.
My taht.net address tends to bounce to vger as I long ago mandated
starttls on all email transactions which is why I use my gmail account
for postings there. I keep hoping that one day vger will support
starttls... posting it here as I'm an optimist. I wasn't expecting to be
cc'd on this submittal....
Pie seems to be mostly an abandoned CISCO effort outside of docsis-pie,
at least at the moment. I've heard not a peep from them in years.
(fq_codel seems to have mostly "won" in our world)
I did review all these changes when they went by in v1, and aside from
the ecn mistake ending up in the final RFC when I wasn't looking[1],
approve of these changes to sch_pie to make it compliant with the rfc,
finally.
[1] We proposed refining pie's ecn handling here:
https://github.com/gautamramk/FQ-PIE-for-Linux-Kernel/issues/2
Anyway:
Acked-by: Dave Taht <dave.taht@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Jamal Hadi Salim @ 2019-02-25 14:33 UTC (permalink / raw)
To: Dave Taht; +Cc: Leslie Monis, netdev, dave.taht
In-Reply-To: <87ftsc2c1e.fsf@taht.net>
On 2019-02-25 9:11 a.m., Dave Taht wrote:
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
>> On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
>>> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
>>>> The current implementation of the PIE queuing discipline is
>>>> according to the
>>>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and
>>>> the paper
>>>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
>>>> However, a lot of necessary modifications and enhancements have
>>>> been proposed
>>>> in RFC 8033, which have not yet been incorporated in the source
>>>> code of Linux.
>>>> This patch series helps in achieving the same.
>>>>
>>>> Performance tests carried out using Flent [https://flent.org/]
>>>>
>>>
>>> +Cc the authors of PIE to double check these values.
>>> Please respond (it is how open source works!) and N/ACK
>>
>> Great. Bouncing addresses.
>> D. Taht - can you look at this?
>
> Whose addresses are bouncing? The folk that did pie originally were
> mostly contractors and long ago moved on to other things.
>
The Cisco addresses - listed as authors of PIE were bouncing.
Note: they are still listed as authors of the referenced
IETF draft with those email addresses.
> My taht.net address tends to bounce to vger as I long ago mandated
> starttls on all email transactions which is why I use my gmail account
> for postings there. I keep hoping that one day vger will support
> starttls... posting it here as I'm an optimist. I wasn't expecting to be
> cc'd on this submittal....
>
> Pie seems to be mostly an abandoned CISCO effort outside of docsis-pie,
> at least at the moment. I've heard not a peep from them in years.
> (fq_codel seems to have mostly "won" in our world)
>
> I did review all these changes when they went by in v1, and aside from
> the ecn mistake ending up in the final RFC when I wasn't looking[1],
> approve of these changes to sch_pie to make it compliant with the rfc,
> finally.
>
> [1] We proposed refining pie's ecn handling here:
>
> https://github.com/gautamramk/FQ-PIE-for-Linux-Kernel/issues/2
>
> Anyway:
>
> Acked-by: Dave Taht <dave.taht@gmail.com>
>
Thats what i was hoping for. FWIW,
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
cheers,
jamal
^ permalink raw reply
* Re: [net-next 05/14] i40e: fix up 32 bit timespec references
From: Arnd Bergmann @ 2019-02-25 14:46 UTC (permalink / raw)
To: Jeff Kirsher
Cc: David Miller, Jesse Brandeburg, Networking, nhorman, sassmann,
jogreene
In-Reply-To: <20170726103123.76743-6-jeffrey.t.kirsher@intel.com>
On Wed, Jul 26, 2017 at 12:33 PM Jeff Kirsher
<jeffrey.t.kirsher@intel.com> wrote:
>
> From: Jesse Brandeburg <jesse.brandeburg@intel.com>
>
> As it turns out there was only a small set of errors
> on 32 bit, and we just needed to be using the right calls
> for dealing with timespec64 variables.
I just stumbled over code added by this older patch, and can't make sense
of the commit description here. Was this an attempt to fix a bug, or
just a cleanup?
>
> - then = ns_to_timespec64(delta);
> mutex_lock(&pf->tmreg_lock);
>
> i40e_ptp_read(pf, &now);
> - now = timespec64_add(now, then);
> + timespec64_add_ns(&now, delta);
> i40e_ptp_write(pf, (const struct timespec64 *)&now);
The problem I noticed here is that 'delta' is a user provided 64-bit
number from clock_adjtime(), and timespec64_add_ns() performs uses
a repeated addition instead of a div/mod pair. When the number
is large, we may end up adding a single second 8 billion times,
which may take a while even on a fast CPU.
Should the commit 0ac30ce43323 ("i40e: fix up 32 bit timespec
references") just be reverted?
Arnd
^ permalink raw reply
* [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
From: Sheng Lan @ 2019-02-25 14:49 UTC (permalink / raw)
To: davem, stephen
Cc: netdev, netem, xuhanbing, zhengshaoyu, jiqin.ji, liuzhiqiang26
From: Sheng Lan <lansheng@huawei.com>
Subject: [PATCH] net: netem: fix skb length BUG_ON in __skb_to_sgvec
It can be reproduced by following steps:
1. virtio_net NIC is configured with gso/tso on
2. configure nginx as http server with an index file bigger than 1M bytes
3. use tc netem to produce duplicate packets and delay:
tc qdisc add dev eth0 root netem delay 100ms 10ms 30% duplicate 90%
4. continually curl the nginx http server to get index file on client
5. BUG_ON is seen quickly
[10258690.371129] kernel BUG at net/core/skbuff.c:4028!
[10258690.371748] invalid opcode: 0000 [#1] SMP PTI
[10258690.372094] CPU: 5 PID: 0 Comm: swapper/5 Tainted: G W 5.0.0-rc6 #2
[10258690.372094] RSP: 0018:ffffa05797b43da0 EFLAGS: 00010202
[10258690.372094] RBP: 00000000000005ea R08: 0000000000000000 R09: 00000000000005ea
[10258690.372094] R10: ffffa0579334d800 R11: 00000000000002c0 R12: 0000000000000002
[10258690.372094] R13: 0000000000000000 R14: ffffa05793122900 R15: ffffa0578f7cb028
[10258690.372094] FS: 0000000000000000(0000) GS:ffffa05797b40000(0000) knlGS:0000000000000000
[10258690.372094] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[10258690.372094] CR2: 00007f1a6dc00868 CR3: 000000001000e000 CR4: 00000000000006e0
[10258690.372094] Call Trace:
[10258690.372094] <IRQ>
[10258690.372094] skb_to_sgvec+0x11/0x40
[10258690.372094] start_xmit+0x38c/0x520 [virtio_net]
[10258690.372094] dev_hard_start_xmit+0x9b/0x200
[10258690.372094] sch_direct_xmit+0xff/0x260
[10258690.372094] __qdisc_run+0x15e/0x4e0
[10258690.372094] net_tx_action+0x137/0x210
[10258690.372094] __do_softirq+0xd6/0x2a9
[10258690.372094] irq_exit+0xde/0xf0
[10258690.372094] smp_apic_timer_interrupt+0x74/0x140
[10258690.372094] apic_timer_interrupt+0xf/0x20
[10258690.372094] </IRQ>
In __skb_to_sgvec, the skb->len is not equal to the sum of the skb's
linear data size and nonlinear data size, thus BUG_ON triggered. The
bad skb's nonlinear data size is less than skb->data_len, because the
skb is cloned and a part of related cloned skb's nonlinear data is
split off.
Duplicate packet is cloned by skb_clone in netem_enqueue and may be delayed
some time in qdisc. Due to the delay time, the original skb will be pushed
again later in __tcp_push_pending_frames when tcp receives new packets.
In tcp_write_xmit, when the tcp_mss_split_point returns a smaller limit,
the original skb will be fragmented and the skb's nonlinear data will be
split off. The length of the skb cloned by netem will not be updated.
When we use virtio_net NIC, the duplicated cloned skb will be filled into
a scatter-gather list in __skb_to_sgvec and trigger the BUG_ON.
Here I replace the skb_clone with skb_copy in netem_enqueue to ensure
the duplicated skb's nonlinear data is independent.
Signed-off-by: Sheng Lan <lansheng@huawei.com>
Reported-by: Qin Ji <jiqin.ji@huawei.com>
Fixes: 0afb51e7 ("netem: reinsert for duplication")
---
net/sched/sch_netem.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_netem.c b/net/sched/sch_netem.c
index 75046ec..76d9740 100644
--- a/net/sched/sch_netem.c
+++ b/net/sched/sch_netem.c
@@ -479,7 +479,7 @@ static int netem_enqueue(struct sk_buff *skb, struct Qdisc *sch,
* qdisc tree, since parent queuer expects that only one
* skb will be queued.
*/
- if (count > 1 && (skb2 = skb_clone(skb, GFP_ATOMIC)) != NULL) {
+ if (count > 1 && (skb2 = skb_copy(skb, GFP_ATOMIC)) != NULL) {
struct Qdisc *rootq = qdisc_root(sch);
u32 dupsave = q->duplicate; /* prevent duplicating a dup... */
--
^ permalink raw reply related
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Marc Zyngier @ 2019-02-25 14:52 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, Devicetree List, <netdev@vger.kernel.org>,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
linux-rockchip, David S. Miller, linux-arm-kernel
In-Reply-To: <CAKv+Gu9OOzOxx0rTAZqM8r4q_hRmOp3b=KP7fwgNeiu67FCApA@mail.gmail.com>
Hi Ard,
On 25/02/2019 12:45, Ard Biesheuvel wrote:
> On Sun, 24 Feb 2019 at 15:08, Marc Zyngier <marc.zyngier@arm.com> wrote:
>>
>> For quite some time, I wondered why the PCI mwifiex device built in my
>> Chromebook was unable to use the good old legacy interrupts. But as MSIs
>> were working fine, I never really bothered investigating. I finally had a
>> look, and the result isn't very pretty.
>>
>> On this machine (rk3399-based kevin), the wake-up interrupt is described as
>> such:
>>
>> &pci_rootport {
>> mvl_wifi: wifi@0,0 {
>> compatible = "pci1b4b,2b42";
>> reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
>> 0x83010000 0x0 0x00100000 0x0 0x00100000>;
>> interrupt-parent = <&gpio0>;
>> interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
>> pinctrl-names = "default";
>> pinctrl-0 = <&wlan_host_wake_l>;
>> wakeup-source;
>> };
>> };
>>
>> Note how the interrupt is part of the properties directly attached to the
>> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
>> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
>> altogether (Yay for the broken design!). This is in total violation of the
>> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
>> specifiers describe the PCI device interrupts, and must obey the
>> INT-{A,B,C,D} mapping. Oops!
>>
>
> The mapping of legacy PCIe INTx interrupts onto wired system
> interrupts is a property of the PCIe host controller, not of a
> particular PCIe device. So I would argue that the code is broken here
> as well: it should never attempt to interpret 'interrupt' properties
> at the PCI device level as having any bearing on how legacy interrupts
> are routed.
OpenFirmware says that this node contains the interrupt number of the
device (4.1.1. Open Firmware-defined Properties for Child Nodes). The
trick is that this property is generated *from* the device, and not set
in stone.
DT, on the other hand, takes whatever is described there and uses it as
the gospel to configure the OS, no matter how the PCI device is actually
configured. If the two don't match (like in this case), things break.
This is the "DT describes the HW" mantra, for (sometimes) better or
(more generally) worse.
What the DT code does is to interpret the whole interrupt specifier,
*including the interrupt-parent*. And that feels wrong. It should always
be in the context of the host controller. But on the other side, the DT
code is not in the business of validating the DT either...
It outlines one thing: If you have to interpret per-device PCI
properties from DT, you're in for serious trouble. I should get some
better HW.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* Re: [PATCH v2 2/2] drivers: net: phy: mdio-mux: Add support for Generic Mux controls
From: Peter Rosin @ 2019-02-25 14:56 UTC (permalink / raw)
To: Andrew Lunn, Pankaj Bansal
Cc: Leo Li, Florian Fainelli, Heiner Kallweit, netdev@vger.kernel.org
In-Reply-To: <20190224145733.GD26626@lunn.ch>
On 2019-02-24 15:57, Andrew Lunn wrote:
>> +static int mdio_mux_multiplexer_remove(struct platform_device *pdev)
>> +{
>> + struct mdio_mux_multiplexer_state *s = platform_get_drvdata(pdev);
>> +
>> + mdio_mux_uninit(s->mux_handle);
>
> I've never used the multiplexer framework before. But i think you need
> a call to mux_control_deselect() here in order to unlock the
> multiplexer. Without that, it will deadlock when you reload the
> module.
That is correct.
Some background; The mux framework was designed for cases where
a) two (or more) mux consumers "compete" for the same mux controller, or
b) where a single mux chip shares several mux controllers where each
controller might be used for some independent purpose.
The design is also built around short accesses, such that no single
mux consumer starves other consumers of a shared mux controller. I.e.
- select the mux and implicitly set it in the desired position
- do something short-ish which requires the mux
- deselect the mux
But it does work for exclusive consumers of a mux controller as in this
series. However, if the consumer
1) requires the implementation of a new mux controller,
2) does not need any of the existing mux drivers and
3) the situation is not in one of the a) and b) categories
then the mux framework is not really providing any killer features. I.e.
it seems like it is certainly an option to just open code the needed
regmap accesses and ignore the multiplexer subsystem in this case.
However, the changes needed in the mux framework are quite minimal in
this case [1], so 1) is arguably not applicable...
More details on the design can be found in the original mux series [2].
Cheers,
Peter
[1] https://lkml.org/lkml/2019/2/24/23
[2] https://lkml.org/lkml/2017/5/14/160
^ permalink raw reply
* [PATCH net-next] net: lmc: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 14:57 UTC (permalink / raw)
To: netdev; +Cc: davem, colin.king, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in lmc_interrupt() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Delete a redundant comment line in lmc_interrupt().
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/lmc/lmc_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wan/lmc/lmc_main.c b/drivers/net/wan/lmc/lmc_main.c
index 4907453..9a8e809 100644
--- a/drivers/net/wan/lmc/lmc_main.c
+++ b/drivers/net/wan/lmc/lmc_main.c
@@ -1320,8 +1320,7 @@ static irqreturn_t lmc_interrupt (int irq, void *dev_instance) /*fold00*/
sc->lmc_device->stats.tx_packets++;
}
- // dev_kfree_skb(sc->lmc_txq[i]);
- dev_kfree_skb_irq(sc->lmc_txq[i]);
+ dev_consume_skb_irq(sc->lmc_txq[i]);
sc->lmc_txq[i] = NULL;
badtx++;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: wan: wanxl: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:01 UTC (permalink / raw)
To: netdev; +Cc: khc, davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in wanxl_tx_intr() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/wanxl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/wanxl.c b/drivers/net/wan/wanxl.c
index d573a57..5c234a6 100644
--- a/drivers/net/wan/wanxl.c
+++ b/drivers/net/wan/wanxl.c
@@ -185,7 +185,7 @@ static inline void wanxl_tx_intr(struct port *port)
desc->stat = PACKET_EMPTY; /* Free descriptor */
pci_unmap_single(port->card->pdev, desc->address, skb->len,
PCI_DMA_TODEVICE);
- dev_kfree_skb_irq(skb);
+ dev_consume_skb_irq(skb);
port->tx_in = (port->tx_in + 1) % TX_BUFFERS;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: wan: ixp4xx_hss: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:02 UTC (permalink / raw)
To: netdev; +Cc: khalasa, davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in hss_hdlc_txdone_irq() when
skb xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/ixp4xx_hss.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/ixp4xx_hss.c b/drivers/net/wan/ixp4xx_hss.c
index 6a505c2..5c60dc6 100644
--- a/drivers/net/wan/ixp4xx_hss.c
+++ b/drivers/net/wan/ixp4xx_hss.c
@@ -246,7 +246,7 @@
#ifdef __ARMEB__
typedef struct sk_buff buffer_t;
#define free_buffer dev_kfree_skb
-#define free_buffer_irq dev_kfree_skb_irq
+#define free_buffer_irq dev_consume_skb_irq
#else
typedef void buffer_t;
#define free_buffer kfree
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: wan: sbni: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:03 UTC (permalink / raw)
To: netdev; +Cc: davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in send_complete() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/sbni.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/sbni.c b/drivers/net/wan/sbni.c
index 8e8c4c0..40c04ea 100644
--- a/drivers/net/wan/sbni.c
+++ b/drivers/net/wan/sbni.c
@@ -761,7 +761,7 @@ send_complete( struct net_device *dev )
dev->stats.tx_packets++;
dev->stats.tx_bytes += nl->tx_buf_p->len;
#endif
- dev_kfree_skb_irq( nl->tx_buf_p );
+ dev_consume_skb_irq(nl->tx_buf_p);
nl->tx_buf_p = NULL;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next v2 0/7] net: sched: pie: align PIE implementation with RFC 8033
From: Leslie Monis @ 2019-02-25 15:04 UTC (permalink / raw)
To: Dave Taht; +Cc: netdev
In-Reply-To: <87ftsc2c1e.fsf@taht.net>
On Mon, Feb 25, 2019 at 06:11:25AM -0800, Dave Taht wrote:
> Jamal Hadi Salim <jhs@mojatatu.com> writes:
>
> > On 2019-02-25 8:43 a.m., Jamal Hadi Salim wrote:
> >> On 2019-02-25 5:20 a.m., Leslie Monis wrote:
> >>> The current implementation of the PIE queuing discipline is
> >>> according to the
> >>> IETF draft [http://tools.ietf.org/html/draft-pan-aqm-pie-00] and
> >>> the paper
> >>> [PIE: A Lightweight Control Scheme to Address the Bufferbloat Problem].
> >>> However, a lot of necessary modifications and enhancements have
> >>> been proposed
> >>> in RFC 8033, which have not yet been incorporated in the source
> >>> code of Linux.
> >>> This patch series helps in achieving the same.
> >>>
> >>> Performance tests carried out using Flent [https://flent.org/]
> >>>
> >>
> >> +Cc the authors of PIE to double check these values.
> >> Please respond (it is how open source works!) and N/ACK
> >
> > Great. Bouncing addresses.
> > D. Taht - can you look at this?
>
> Whose addresses are bouncing? The folk that did pie originally were
> mostly contractors and long ago moved on to other things.
>
> My taht.net address tends to bounce to vger as I long ago mandated
> starttls on all email transactions which is why I use my gmail account
> for postings there. I keep hoping that one day vger will support
> starttls... posting it here as I'm an optimist. I wasn't expecting to be
> cc'd on this submittal....
>
> Pie seems to be mostly an abandoned CISCO effort outside of docsis-pie,
> at least at the moment. I've heard not a peep from them in years.
> (fq_codel seems to have mostly "won" in our world)
>
> I did review all these changes when they went by in v1, and aside from
> the ecn mistake ending up in the final RFC when I wasn't looking[1],
> approve of these changes to sch_pie to make it compliant with the rfc,
> finally.
>
> [1] We proposed refining pie's ecn handling here:
>
> https://github.com/gautamramk/FQ-PIE-for-Linux-Kernel/issues/2
>
> Anyway:
>
> Acked-by: Dave Taht <dave.taht@gmail.com>
>
Hi Dave,
For now we would just like to make the PIE implementation meet the
standards as expressed by the RFC. As ECN handling is outside the
scope of the RFC we can consider refinements to it in the code later.
Thanks,
Leslie
^ permalink raw reply
* [PATCH net-next] net: wan: cosa: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:05 UTC (permalink / raw)
To: netdev; +Cc: kas, davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in cosa_net_tx_done() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/cosa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/cosa.c b/drivers/net/wan/cosa.c
index f6b000d..55f76e4 100644
--- a/drivers/net/wan/cosa.c
+++ b/drivers/net/wan/cosa.c
@@ -769,7 +769,7 @@ static int cosa_net_tx_done(struct channel_data *chan, int size)
chan->netdev->stats.tx_aborted_errors++;
return 1;
}
- dev_kfree_skb_irq(chan->tx_skb);
+ dev_consume_skb_irq(chan->tx_skb);
chan->tx_skb = NULL;
chan->netdev->stats.tx_packets++;
chan->netdev->stats.tx_bytes += size;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] net: wan: z85230: replace dev_kfree_skb_irq by dev_consume_skb_irq for drop profiles
From: Yang Wei @ 2019-02-25 15:06 UTC (permalink / raw)
To: netdev; +Cc: davem, yang.wei9, albin_yang
From: Yang Wei <yang.wei9@zte.com.cn>
dev_consume_skb_irq() should be called in z8530_tx_done() when skb
xmit done. It makes drop profiles(dropwatch, perf) more friendly.
Signed-off-by: Yang Wei <yang.wei9@zte.com.cn>
---
drivers/net/wan/z85230.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/z85230.c b/drivers/net/wan/z85230.c
index deea41e..0e56ab0 100644
--- a/drivers/net/wan/z85230.c
+++ b/drivers/net/wan/z85230.c
@@ -1536,7 +1536,7 @@ static void z8530_tx_done(struct z8530_channel *c)
z8530_tx_begin(c);
c->netdevice->stats.tx_packets++;
c->netdevice->stats.tx_bytes += skb->len;
- dev_kfree_skb_irq(skb);
+ dev_consume_skb_irq(skb);
}
/**
--
2.7.4
^ permalink raw reply related
* [PATCH net v3 0/2] selftests: pmtu: fix and increase coverage
From: Paolo Abeni @ 2019-02-25 15:08 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Stefano Brivio, Sabrina Dubroca
This series includes a fixup for the pmtu.sh test script, related to IPv6
address management, and adds coverage for the recently reported and fixed
PMTU exception issue
v2 -> v3:
- more cleanups
v1 -> v2:
- several script cleanups
Paolo Abeni (2):
selftests: pmtu: disable DAD in all namespaces
selftests: pmtu: add explicit tests for PMTU exceptions cleanup
tools/testing/selftests/net/pmtu.sh | 96 ++++++++++++++++++++++++-----
1 file changed, 80 insertions(+), 16 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH net v3 2/2] selftests: pmtu: add explicit tests for PMTU exceptions cleanup
From: Paolo Abeni @ 2019-02-25 15:08 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Stefano Brivio, Sabrina Dubroca
In-Reply-To: <cover.1551106591.git.pabeni@redhat.com>
Add a couple of new tests, explicitly checking that the kernel
timely releases PMTU exceptions on related device removal.
This is mostly a regression test vs the issue fixed by
commit f5b51fe804ec ("ipv6: route: purge exception on removal")
Only 2 new test cases have been added, instead of extending all
the existing ones, because the reproducer requires executing
several commands and would slow down too much the tests otherwise.
v2 -> v3:
- more cleanup, still from Stefano
v1 -> v2:
- several script cleanups, as suggested by Stefano
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
tools/testing/selftests/net/pmtu.sh | 68 ++++++++++++++++++++++++++++-
1 file changed, 67 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
index 89aec2fdf4fa..912b2dc50be3 100755
--- a/tools/testing/selftests/net/pmtu.sh
+++ b/tools/testing/selftests/net/pmtu.sh
@@ -103,6 +103,15 @@
# and check that configured MTU is used on link creation and changes, and
# that MTU is properly calculated instead when MTU is not configured from
# userspace
+#
+# - cleanup_ipv4_exception
+# Similar to pmtu_ipv4_vxlan4_exception, but explicitly generate PMTU
+# exceptions on multiple CPUs and check that the veth device tear-down
+# happens in a timely manner
+#
+# - cleanup_ipv6_exception
+# Same as above, but use IPv6 transport from A to B
+
# Kselftest framework requirement - SKIP code is 4.
ksft_skip=4
@@ -135,7 +144,9 @@ tests="
pmtu_vti6_default_mtu vti6: default MTU assignment
pmtu_vti4_link_add_mtu vti4: MTU setting on link creation
pmtu_vti6_link_add_mtu vti6: MTU setting on link creation
- pmtu_vti6_link_change_mtu vti6: MTU changes on link changes"
+ pmtu_vti6_link_change_mtu vti6: MTU changes on link changes
+ cleanup_ipv4_exception ipv4: cleanup of cached exceptions
+ cleanup_ipv6_exception ipv6: cleanup of cached exceptions"
NS_A="ns-$(mktemp -u XXXXXX)"
NS_B="ns-$(mktemp -u XXXXXX)"
@@ -1006,6 +1017,61 @@ test_pmtu_vti6_link_change_mtu() {
return ${fail}
}
+check_command() {
+ cmd=${1}
+
+ if ! which ${cmd} > /dev/null 2>&1; then
+ err " missing required command: '${cmd}'"
+ return 1
+ fi
+ return 0
+}
+
+test_cleanup_vxlanX_exception() {
+ outer="${1}"
+ encap="vxlan"
+ ll_mtu=4000
+
+ check_command taskset || return 2
+ cpu_list=$(grep -m 2 processor /proc/cpuinfo | cut -d ' ' -f 2)
+
+ setup namespaces routing ${encap}${outer} || return 2
+ trace "${ns_a}" ${encap}_a "${ns_b}" ${encap}_b \
+ "${ns_a}" veth_A-R1 "${ns_r1}" veth_R1-A \
+ "${ns_b}" veth_B-R1 "${ns_r1}" veth_R1-B
+
+ # Create route exception by exceeding link layer MTU
+ mtu "${ns_a}" veth_A-R1 $((${ll_mtu} + 1000))
+ mtu "${ns_r1}" veth_R1-A $((${ll_mtu} + 1000))
+ mtu "${ns_b}" veth_B-R1 ${ll_mtu}
+ mtu "${ns_r1}" veth_R1-B ${ll_mtu}
+
+ mtu "${ns_a}" ${encap}_a $((${ll_mtu} + 1000))
+ mtu "${ns_b}" ${encap}_b $((${ll_mtu} + 1000))
+
+ # Fill exception cache for multiple CPUs (2)
+ # we can always use inner IPv4 for that
+ for cpu in ${cpu_list}; do
+ taskset --cpu-list ${cpu} ${ns_a} ping -q -M want -i 0.1 -w 1 -s $((${ll_mtu} + 500)) ${tunnel4_b_addr} > /dev/null
+ done
+
+ ${ns_a} ip link del dev veth_A-R1 &
+ iplink_pid=$!
+ sleep 1
+ if [ "$(cat /proc/${iplink_pid}/cmdline 2>/dev/null | tr -d '\0')" = "iplinkdeldevveth_A-R1" ]; then
+ err " can't delete veth device in a timely manner, PMTU dst likely leaked"
+ return 1
+ fi
+}
+
+test_cleanup_ipv6_exception() {
+ test_cleanup_vxlanX_exception 6
+}
+
+test_cleanup_ipv4_exception() {
+ test_cleanup_vxlanX_exception 4
+}
+
usage() {
echo
echo "$0 [OPTIONS] [TEST]..."
--
2.20.1
^ permalink raw reply related
* [PATCH net v3 1/2] selftests: pmtu: disable DAD in all namespaces
From: Paolo Abeni @ 2019-02-25 15:08 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, David Ahern, Stefano Brivio, Sabrina Dubroca
In-Reply-To: <cover.1551106591.git.pabeni@redhat.com>
Otherwise, the configured IPv6 address could be still "tentative"
at test time, possibly causing tests failures.
We can also drop some sleep along the code and decrease the
timeout for most commands so that the test runtime decreases.
v1 -> v2:
- fix comment (Stefano)
Fixes: d1f1b9cbf34c ("selftests: net: Introduce first PMTU test")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
Reviewed-by: David Ahern <dsahern@gmail.com>
---
tools/testing/selftests/net/pmtu.sh | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/net/pmtu.sh b/tools/testing/selftests/net/pmtu.sh
index e2c94e47707c..89aec2fdf4fa 100755
--- a/tools/testing/selftests/net/pmtu.sh
+++ b/tools/testing/selftests/net/pmtu.sh
@@ -263,8 +263,6 @@ setup_fou_or_gue() {
${ns_a} ip link set ${encap}_a up
${ns_b} ip link set ${encap}_b up
-
- sleep 1
}
setup_fou44() {
@@ -302,6 +300,10 @@ setup_gue66() {
setup_namespaces() {
for n in ${NS_A} ${NS_B} ${NS_R1} ${NS_R2}; do
ip netns add ${n} || return 1
+
+ # Disable DAD, so that we don't have to wait to use the
+ # configured IPv6 addresses
+ ip netns exec ${n} sysctl -q net/ipv6/conf/default/accept_dad=0
done
}
@@ -337,8 +339,6 @@ setup_vti() {
${ns_a} ip link set vti${proto}_a up
${ns_b} ip link set vti${proto}_b up
-
- sleep 1
}
setup_vti4() {
@@ -375,8 +375,6 @@ setup_vxlan_or_geneve() {
${ns_a} ip link set ${type}_a up
${ns_b} ip link set ${type}_b up
-
- sleep 1
}
setup_geneve4() {
@@ -588,8 +586,8 @@ test_pmtu_ipvX() {
mtu "${ns_b}" veth_B-R2 1500
# Create route exceptions
- ${ns_a} ${ping} -q -M want -i 0.1 -w 2 -s 1800 ${dst1} > /dev/null
- ${ns_a} ${ping} -q -M want -i 0.1 -w 2 -s 1800 ${dst2} > /dev/null
+ ${ns_a} ${ping} -q -M want -i 0.1 -w 1 -s 1800 ${dst1} > /dev/null
+ ${ns_a} ${ping} -q -M want -i 0.1 -w 1 -s 1800 ${dst2} > /dev/null
# Check that exceptions have been created with the correct PMTU
pmtu_1="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst1})"
@@ -621,7 +619,7 @@ test_pmtu_ipvX() {
# Decrease remote MTU on path via R2, get new exception
mtu "${ns_r2}" veth_R2-B 400
mtu "${ns_b}" veth_B-R2 400
- ${ns_a} ${ping} -q -M want -i 0.1 -w 2 -s 1400 ${dst2} > /dev/null
+ ${ns_a} ${ping} -q -M want -i 0.1 -w 1 -s 1400 ${dst2} > /dev/null
pmtu_2="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst2})"
check_pmtu_value "lock 552" "${pmtu_2}" "exceeding MTU, with MTU < min_pmtu" || return 1
@@ -638,7 +636,7 @@ test_pmtu_ipvX() {
check_pmtu_value "1500" "${pmtu_2}" "increasing local MTU" || return 1
# Get new exception
- ${ns_a} ${ping} -q -M want -i 0.1 -w 2 -s 1400 ${dst2} > /dev/null
+ ${ns_a} ${ping} -q -M want -i 0.1 -w 1 -s 1400 ${dst2} > /dev/null
pmtu_2="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst2})"
check_pmtu_value "lock 552" "${pmtu_2}" "exceeding MTU, with MTU < min_pmtu" || return 1
}
@@ -687,7 +685,7 @@ test_pmtu_ipvX_over_vxlanY_or_geneveY_exception() {
mtu "${ns_a}" ${type}_a $((${ll_mtu} + 1000))
mtu "${ns_b}" ${type}_b $((${ll_mtu} + 1000))
- ${ns_a} ${ping} -q -M want -i 0.1 -w 2 -s $((${ll_mtu} + 500)) ${dst} > /dev/null
+ ${ns_a} ${ping} -q -M want -i 0.1 -w 1 -s $((${ll_mtu} + 500)) ${dst} > /dev/null
# Check that exception was created
pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst})"
@@ -767,7 +765,7 @@ test_pmtu_ipvX_over_fouY_or_gueY() {
mtu "${ns_a}" ${encap}_a $((${ll_mtu} + 1000))
mtu "${ns_b}" ${encap}_b $((${ll_mtu} + 1000))
- ${ns_a} ${ping} -q -M want -i 0.1 -w 2 -s $((${ll_mtu} + 500)) ${dst} > /dev/null
+ ${ns_a} ${ping} -q -M want -i 0.1 -w 1 -s $((${ll_mtu} + 500)) ${dst} > /dev/null
# Check that exception was created
pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${dst})"
@@ -825,13 +823,13 @@ test_pmtu_vti4_exception() {
# Send DF packet without exceeding link layer MTU, check that no
# exception is created
- ${ns_a} ping -q -M want -i 0.1 -w 2 -s ${ping_payload} ${tunnel4_b_addr} > /dev/null
+ ${ns_a} ping -q -M want -i 0.1 -w 1 -s ${ping_payload} ${tunnel4_b_addr} > /dev/null
pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${tunnel4_b_addr})"
check_pmtu_value "" "${pmtu}" "sending packet smaller than PMTU (IP payload length ${esp_payload_rfc4106})" || return 1
# Now exceed link layer MTU by one byte, check that exception is created
# with the right PMTU value
- ${ns_a} ping -q -M want -i 0.1 -w 2 -s $((ping_payload + 1)) ${tunnel4_b_addr} > /dev/null
+ ${ns_a} ping -q -M want -i 0.1 -w 1 -s $((ping_payload + 1)) ${tunnel4_b_addr} > /dev/null
pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${tunnel4_b_addr})"
check_pmtu_value "${esp_payload_rfc4106}" "${pmtu}" "exceeding PMTU (IP payload length $((esp_payload_rfc4106 + 1)))"
}
@@ -847,7 +845,7 @@ test_pmtu_vti6_exception() {
mtu "${ns_b}" veth_b 4000
mtu "${ns_a}" vti6_a 5000
mtu "${ns_b}" vti6_b 5000
- ${ns_a} ${ping6} -q -i 0.1 -w 2 -s 60000 ${tunnel6_b_addr} > /dev/null
+ ${ns_a} ${ping6} -q -i 0.1 -w 1 -s 60000 ${tunnel6_b_addr} > /dev/null
# Check that exception was created
pmtu="$(route_get_dst_pmtu_from_exception "${ns_a}" ${tunnel6_b_addr})"
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next] net: sched: act_tunnel_key: fix metadata handling
From: Vlad Buslov @ 2019-02-25 15:12 UTC (permalink / raw)
To: Davide Caratti
Cc: netdev@vger.kernel.org, jhs@mojatatu.com,
xiyou.wangcong@gmail.com, jiri@resnulli.us, davem@davemloft.net,
wenxu@ucloud.cn, Roi Dayan
In-Reply-To: <4bde1d403d4ba9b51cf18bbaac1d46147011b959.camel@redhat.com>
On Mon 25 Feb 2019 at 14:04, Davide Caratti <dcaratti@redhat.com> wrote:
> On Mon, 2019-02-25 at 14:21 +0200, Vlad Buslov wrote:
>> Tunnel key action params->tcft_enc_metadata is only set when action is
>> TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
>> dereferenced during tunnel key init and release without verifying that
>> action is if correct type, which causes NULL pointer dereference. Metadata
>> tunnel dst_cache is also leaked on action overwrite.
>>
>> Fix metadata handling:
>> - Verify that metadata pointer is not NULL before dereferencing it in
>> tunnel_key_init error handling code.
>
> hello Vlad,
>
> thanks a lot for fixing this!
>
> <...>
>
>> @@ -384,10 +390,12 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
>>
>> release_dst_cache:
>> #ifdef CONFIG_DST_CACHE
>> - dst_cache_destroy(&metadata->u.tun_info.dst_cache);
>> + if (metadata)
>> + dst_cache_destroy(&metadata->u.tun_info.dst_cache);
>> #endif
>> release_tun_meta:
>> - dst_release(&metadata->dst);
>> + if (metadata)
>> + dst_release(&metadata->dst);
>
> on Linux 'net' tree we don't have commit 41411e2fd6b8 ("net/sched:
> act_tunnel_key: Add dst_cache support"), but still the above two lines can
> avoid a NULL dereference in tunnel_key_init() error path, in the following
> case:
>
> * create an action with tunnel "set", with success
> * replace the previous rule rule with tunnel "unset", and have a failure
> here (e.g. allocation of 'params_new').
>
> At the cost of creating some conflicts during the merge, it would probably
> be safer to split this commit into two parts, one targeting 'net' and one
> targeting 'net-next', so that the first one can be proposed for stable
> backports (and also I can rebase/retest my 'goto chain' series on top of
> it :) )
>
> WDYT?
Makes sense. I'll send split patches.
^ permalink raw reply
* Re: [PATCH net] ipvs: get sctphdr by sctphoff in sctp_csum_check
From: Marcelo Ricardo Leitner @ 2019-02-25 15:28 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, netfilter-devel, Neil Horman, pablo
In-Reply-To: <cd1c829cfd9ee916e57c51f6879947f0f88c1eb1.1551094063.git.lucien.xin@gmail.com>
On Mon, Feb 25, 2019 at 07:27:43PM +0800, Xin Long wrote:
> sctp_csum_check() is called by sctp_s/dnat_handler() where it calls
> skb_make_writable() to ensure sctphdr to be linearized.
>
> So there's no need to get sctphdr by calling skb_header_pointer()
> in sctp_csum_check().
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/netfilter/ipvs/ip_vs_proto_sctp.c | 7 ++-----
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_proto_sctp.c b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> index b0cd7d0..0ecf241 100644
> --- a/net/netfilter/ipvs/ip_vs_proto_sctp.c
> +++ b/net/netfilter/ipvs/ip_vs_proto_sctp.c
> @@ -183,7 +183,7 @@ static int
> sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp)
> {
> unsigned int sctphoff;
> - struct sctphdr *sh, _sctph;
> + struct sctphdr *sh;
> __le32 cmp, val;
>
> #ifdef CONFIG_IP_VS_IPV6
> @@ -193,10 +193,7 @@ sctp_csum_check(int af, struct sk_buff *skb, struct ip_vs_protocol *pp)
> #endif
> sctphoff = ip_hdrlen(skb);
>
> - sh = skb_header_pointer(skb, sctphoff, sizeof(_sctph), &_sctph);
> - if (sh == NULL)
> - return 0;
> -
> + sh = (struct sctphdr *)(skb->data + sctphoff);
> cmp = sh->checksum;
> val = sctp_compute_cksum(skb, sctphoff);
>
> --
> 2.1.0
>
^ permalink raw reply
* [PATCH net] net: sched: act_tunnel_key: fix NULL pointer dereference during init
From: Vlad Buslov @ 2019-02-25 15:28 UTC (permalink / raw)
To: netdev, dcaratti
Cc: jhs, xiyou.wangcong, jiri, davem, wenxu, roid, Vlad Buslov
In-Reply-To: <4bde1d403d4ba9b51cf18bbaac1d46147011b959.camel@redhat.com>
Metadata pointer is only initialized for action TCA_TUNNEL_KEY_ACT_SET, but
it is unconditionally dereferenced in tunnel_key_init() error handler.
Verify that metadata pointer is not NULL before dereferencing it in
tunnel_key_init error handling code.
Fixes: ee28bb56ac5b ("net/sched: fix memory leak in act_tunnel_key_init()")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/act_tunnel_key.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 8b43fe0130f7..3f943de9a2c9 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -377,7 +377,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
return ret;
release_tun_meta:
- dst_release(&metadata->dst);
+ if (metadata)
+ dst_release(&metadata->dst);
err_out:
if (exists)
--
2.13.6
^ permalink raw reply related
* Re: [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode
From: Tariq Toukan @ 2019-02-25 15:30 UTC (permalink / raw)
To: Andrew Lunn, Michal Kubecek
Cc: Tariq Toukan, John W. Linville, netdev@vger.kernel.org,
Eran Ben Elisha, Aya Levin
In-Reply-To: <20190224194015.GM26626@lunn.ch>
On 2/24/2019 9:40 PM, Andrew Lunn wrote:
>>> This is getting less friendly all the time, and it was never very
>>> friendly to start with. We have the strings which represent these link
>>> modes in the table used for dumping caps. How about allowing the user
>>> to list a comma separate list of modes.
>>>
>>> ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full
>>
>> In my preliminary netlink patchset, I'm adding support for e.g.
>>
>> ethtool -s eth0 advertise 100baseT/Half off 1000baseT/Full on
>>
>> I'm not sure what would be more useful, if switching individual modes or
>> listing the whole set. After all, we can also support both. But I fully
>> agree that the numeric bitmaps are already too inconvenient.
>
> Hi Michal
>
> So are you doing a read/modify/write? In that case, off/on makes
> sense. For a pure write, i don't see the need for off/on.
>
> I've not had to use this much, so i don't know how it is typically
> used. When i have used it, it is because i've got an SFP which can do
> 1G and 2.5G, but the peer can only do 1G. I've needed to remove the
> 2.5G in order to get link. So in that case, read/modify/write with an
> off would make sense.
>
> Andrew
>
>> And there is another problem:
>>
>> + else if (speed_wanted == SPEED_20000 &&
>> + duplex_wanted == DUPLEX_FULL)
>> + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT &
>> + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT &
>> + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT &
>> + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT &
>> + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT;
>>
>> The test is for SPEED_20000 but then 200G modes are added.
>
> Oh, yes. Easy to miss. Maybe we should consider adding aliases,
> #define SPEED_200G SPEED_200000, and
Totally agree. This will help.
>
> #define ETHTOOL_LINK_MODE_200GbaseCR4_Full_BIT ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT
In case you do it please consider adding an underscore after the speed,
so it becomes
ETHTOOL_LINK_MODE_200G_baseCR4_Full_BIT.
I think it's more convenient for the human eye.
^ permalink raw reply
* Re: [stable 3.18 backport v2] netlink: Trim skb to alloc size to avoid MSG_TRUNC
From: Greg Kroah-Hartman @ 2019-02-25 15:30 UTC (permalink / raw)
To: Mark Salyzyn
Cc: linux-kernel, Arad, Ronen, David S . Miller, Dmitry Safonov,
David Ahern, Kirill Tkhai, Andrei Vagin, Li RongQing, YU Bo,
Denys Vlasenko, netdev, stable, Eric Dumazet, Alexander Potapenko
In-Reply-To: <20190222160330.34237-1-salyzyn@android.com>
On Fri, Feb 22, 2019 at 08:03:28AM -0800, Mark Salyzyn wrote:
> From: "Arad, Ronen" <ronen.arad@intel.com>
>
> Direct this upstream db65a3aaf29ecce2e34271d52e8d2336b97bd9fe sha to
> stable 3.18. This patch addresses a race condition where a call to
>
> nlk->max_recvmsg_len = max(nlk->max_recvmsg_len, len);
> nlk->max_recvmsg_len = min_t(size_t, nlk->max_recvmsg_len,
>
> one thread in-between another thread:
>
> skb = netlink_alloc_skb(sk,
>
> and
>
> skb_reserve(skb, skb_tailroom(skb) -
> nlk->max_recvmsg_len);
>
> in netlink_dump. The result can be a negative value and will cause
> a kernel panic ad BUG at net/core/skbuff.c because the negative value
> turns into an extremely large positive value.
>
> Original commit:
>
> netlink_dump() allocates skb based on the calculated min_dump_alloc or
> a per socket max_recvmsg_len.
> min_alloc_size is maximum space required for any single netdev
> attributes as calculated by rtnl_calcit().
> max_recvmsg_len tracks the user provided buffer to netlink_recvmsg.
> It is capped at 16KiB.
> The intention is to avoid small allocations and to minimize the number
> of calls required to obtain dump information for all net devices.
>
> netlink_dump packs as many small messages as could fit within an skb
> that was sized for the largest single netdev information. The actual
> space available within an skb is larger than what is requested. It could
> be much larger and up to near 2x with align to next power of 2 approach.
>
> Allowing netlink_dump to use all the space available within the
> allocated skb increases the buffer size a user has to provide to avoid
> truncaion (i.e. MSG_TRUNG flag set).
>
> It was observed that with many VLANs configured on at least one netdev,
> a larger buffer of near 64KiB was necessary to avoid "Message truncated"
> error in "ip link" or "bridge [-c[ompressvlans]] vlan show" when
> min_alloc_size was only little over 32KiB.
>
> This patch trims skb to allocated size in order to allow the user to
> avoid truncation with more reasonable buffer size.
>
> Signed-off-by: Ronen Arad <ronen.arad@intel.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> (cherry pick commit db65a3aaf29ecce2e34271d52e8d2336b97bd9fe)
> Signed-off-by: Mark Salyzyn <salyzyn@android.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Ronen Arad <ronen.arad@intel.com>
> Cc: "David S . Miller" <davem@davemloft.net>
> Cc: Dmitry Safonov <dima@arista.com>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Kirill Tkhai <ktkhai@virtuozzo.com>
> Cc: Andrei Vagin <avagin@virtuozzo.com>
> Cc: Li RongQing <lirongqing@baidu.com>
> Cc: YU Bo <tsu.yubo@gmail.com>
> Cc: Denys Vlasenko <dvlasenk@redhat.com>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org # 3.18
> ---
> net/netlink/af_netlink.c | 34 ++++++++++++++++++++++------------
> 1 file changed, 22 insertions(+), 12 deletions(-)
Now queued up, thanks.
greg k-h
^ permalink raw reply
* [PATCH net-next v2] net: sched: act_tunnel_key: fix metadata handling
From: Vlad Buslov @ 2019-02-25 15:30 UTC (permalink / raw)
To: netdev, dcaratti
Cc: jhs, xiyou.wangcong, jiri, davem, wenxu, roid, Vlad Buslov
In-Reply-To: <4bde1d403d4ba9b51cf18bbaac1d46147011b959.camel@redhat.com>
Tunnel key action params->tcft_enc_metadata is only set when action is
TCA_TUNNEL_KEY_ACT_SET. However, metadata pointer is incorrectly
dereferenced during tunnel key init and release without verifying that
action is if correct type, which causes NULL pointer dereference. Metadata
tunnel dst_cache is also leaked on action overwrite.
Fix metadata handling:
- Verify that metadata pointer is not NULL before dereferencing it in
tunnel_key_init error handling code.
- Move dst_cache destroy code into tunnel_key_release_params() function
that is called in both action overwrite and release cases (fixes resource
leak) and verifies that actions has correct type before dereferencing
metadata pointer (fixes NULL pointer dereference).
Oops with KASAN enabled during tdc tests execution:
[ 261.080482] ==================================================================
[ 261.088049] BUG: KASAN: null-ptr-deref in dst_cache_destroy+0x21/0xa0
[ 261.094613] Read of size 8 at addr 00000000000000b0 by task tc/2976
[ 261.102524] CPU: 14 PID: 2976 Comm: tc Not tainted 5.0.0-rc7+ #157
[ 261.108844] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 261.116726] Call Trace:
[ 261.119234] dump_stack+0x9a/0xeb
[ 261.122625] ? dst_cache_destroy+0x21/0xa0
[ 261.126818] ? dst_cache_destroy+0x21/0xa0
[ 261.131004] kasan_report+0x176/0x192
[ 261.134752] ? idr_get_next+0xd0/0x120
[ 261.138578] ? dst_cache_destroy+0x21/0xa0
[ 261.142768] dst_cache_destroy+0x21/0xa0
[ 261.146799] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
[ 261.152392] tcf_action_cleanup+0x2c/0xc0
[ 261.156490] tcf_generic_walker+0x4c2/0x5c0
[ 261.160794] ? tcf_action_dump_1+0x390/0x390
[ 261.165163] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
[ 261.170865] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
[ 261.176641] tca_action_gd+0x600/0xa40
[ 261.180482] ? tca_get_fill.constprop.17+0x200/0x200
[ 261.185548] ? __lock_acquire+0x588/0x1d20
[ 261.189741] ? __lock_acquire+0x588/0x1d20
[ 261.193922] ? mark_held_locks+0x90/0x90
[ 261.197944] ? mark_held_locks+0x90/0x90
[ 261.202018] ? __nla_parse+0xfe/0x190
[ 261.205774] tc_ctl_action+0x218/0x230
[ 261.209614] ? tcf_action_add+0x230/0x230
[ 261.213726] rtnetlink_rcv_msg+0x3a5/0x600
[ 261.217910] ? lock_downgrade+0x2d0/0x2d0
[ 261.222006] ? validate_linkmsg+0x400/0x400
[ 261.226278] ? find_held_lock+0x6d/0xd0
[ 261.230200] ? match_held_lock+0x1b/0x210
[ 261.234296] ? validate_linkmsg+0x400/0x400
[ 261.238567] netlink_rcv_skb+0xc7/0x1f0
[ 261.242489] ? netlink_ack+0x470/0x470
[ 261.246319] ? netlink_deliver_tap+0x1f3/0x5a0
[ 261.250874] netlink_unicast+0x2ae/0x350
[ 261.254884] ? netlink_attachskb+0x340/0x340
[ 261.261647] ? _copy_from_iter_full+0xdd/0x380
[ 261.268576] ? __virt_addr_valid+0xb6/0xf0
[ 261.275227] ? __check_object_size+0x159/0x240
[ 261.282184] netlink_sendmsg+0x4d3/0x630
[ 261.288572] ? netlink_unicast+0x350/0x350
[ 261.295132] ? netlink_unicast+0x350/0x350
[ 261.301608] sock_sendmsg+0x6d/0x80
[ 261.307467] ___sys_sendmsg+0x48e/0x540
[ 261.313633] ? copy_msghdr_from_user+0x210/0x210
[ 261.320545] ? save_stack+0x89/0xb0
[ 261.326289] ? __lock_acquire+0x588/0x1d20
[ 261.332605] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 261.340063] ? mark_held_locks+0x90/0x90
[ 261.346162] ? do_filp_open+0x138/0x1d0
[ 261.352108] ? may_open_dev+0x50/0x50
[ 261.357897] ? match_held_lock+0x1b/0x210
[ 261.364016] ? __fget_light+0xa6/0xe0
[ 261.369840] ? __sys_sendmsg+0xd2/0x150
[ 261.375814] __sys_sendmsg+0xd2/0x150
[ 261.381610] ? __ia32_sys_shutdown+0x30/0x30
[ 261.388026] ? lock_downgrade+0x2d0/0x2d0
[ 261.394182] ? mark_held_locks+0x1c/0x90
[ 261.400230] ? do_syscall_64+0x1e/0x280
[ 261.406172] do_syscall_64+0x78/0x280
[ 261.411932] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 261.419103] RIP: 0033:0x7f28e91a8b87
[ 261.424791] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
[ 261.448226] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 261.458183] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
[ 261.467728] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
[ 261.477342] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
[ 261.486970] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
[ 261.496599] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
[ 261.506281] ==================================================================
[ 261.516076] Disabling lock debugging due to kernel taint
[ 261.523979] BUG: unable to handle kernel NULL pointer dereference at 00000000000000b0
[ 261.534413] #PF error: [normal kernel read fault]
[ 261.541730] PGD 8000000317400067 P4D 8000000317400067 PUD 316878067 PMD 0
[ 261.551294] Oops: 0000 [#1] SMP KASAN PTI
[ 261.557985] CPU: 14 PID: 2976 Comm: tc Tainted: G B 5.0.0-rc7+ #157
[ 261.568306] Hardware name: Supermicro SYS-2028TP-DECR/X10DRT-P, BIOS 2.0b 03/30/2017
[ 261.578874] RIP: 0010:dst_cache_destroy+0x21/0xa0
[ 261.586413] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
[ 261.611247] RSP: 0018:ffff888316447160 EFLAGS: 00010282
[ 261.619564] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
[ 261.629862] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
[ 261.640149] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
[ 261.650467] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
[ 261.660785] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
[ 261.671110] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
[ 261.682447] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 261.691491] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
[ 261.701283] Call Trace:
[ 261.706374] tunnel_key_release+0x3a/0x50 [act_tunnel_key]
[ 261.714522] tcf_action_cleanup+0x2c/0xc0
[ 261.721208] tcf_generic_walker+0x4c2/0x5c0
[ 261.728074] ? tcf_action_dump_1+0x390/0x390
[ 261.734996] ? tunnel_key_walker+0x5/0x1a0 [act_tunnel_key]
[ 261.743247] ? tunnel_key_walker+0xe9/0x1a0 [act_tunnel_key]
[ 261.751557] tca_action_gd+0x600/0xa40
[ 261.757991] ? tca_get_fill.constprop.17+0x200/0x200
[ 261.765644] ? __lock_acquire+0x588/0x1d20
[ 261.772461] ? __lock_acquire+0x588/0x1d20
[ 261.779266] ? mark_held_locks+0x90/0x90
[ 261.785880] ? mark_held_locks+0x90/0x90
[ 261.792470] ? __nla_parse+0xfe/0x190
[ 261.798738] tc_ctl_action+0x218/0x230
[ 261.805145] ? tcf_action_add+0x230/0x230
[ 261.811760] rtnetlink_rcv_msg+0x3a5/0x600
[ 261.818564] ? lock_downgrade+0x2d0/0x2d0
[ 261.825433] ? validate_linkmsg+0x400/0x400
[ 261.832256] ? find_held_lock+0x6d/0xd0
[ 261.838624] ? match_held_lock+0x1b/0x210
[ 261.845142] ? validate_linkmsg+0x400/0x400
[ 261.851729] netlink_rcv_skb+0xc7/0x1f0
[ 261.857976] ? netlink_ack+0x470/0x470
[ 261.864132] ? netlink_deliver_tap+0x1f3/0x5a0
[ 261.870969] netlink_unicast+0x2ae/0x350
[ 261.877294] ? netlink_attachskb+0x340/0x340
[ 261.883962] ? _copy_from_iter_full+0xdd/0x380
[ 261.890750] ? __virt_addr_valid+0xb6/0xf0
[ 261.897188] ? __check_object_size+0x159/0x240
[ 261.903928] netlink_sendmsg+0x4d3/0x630
[ 261.910112] ? netlink_unicast+0x350/0x350
[ 261.916410] ? netlink_unicast+0x350/0x350
[ 261.922656] sock_sendmsg+0x6d/0x80
[ 261.928257] ___sys_sendmsg+0x48e/0x540
[ 261.934183] ? copy_msghdr_from_user+0x210/0x210
[ 261.940865] ? save_stack+0x89/0xb0
[ 261.946355] ? __lock_acquire+0x588/0x1d20
[ 261.952358] ? entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 261.959468] ? mark_held_locks+0x90/0x90
[ 261.965248] ? do_filp_open+0x138/0x1d0
[ 261.970910] ? may_open_dev+0x50/0x50
[ 261.976386] ? match_held_lock+0x1b/0x210
[ 261.982210] ? __fget_light+0xa6/0xe0
[ 261.987648] ? __sys_sendmsg+0xd2/0x150
[ 261.993263] __sys_sendmsg+0xd2/0x150
[ 261.998613] ? __ia32_sys_shutdown+0x30/0x30
[ 262.004555] ? lock_downgrade+0x2d0/0x2d0
[ 262.010236] ? mark_held_locks+0x1c/0x90
[ 262.015758] ? do_syscall_64+0x1e/0x280
[ 262.021234] do_syscall_64+0x78/0x280
[ 262.026500] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 262.033207] RIP: 0033:0x7f28e91a8b87
[ 262.038421] Code: 64 89 02 48 c7 c0 ff ff ff ff eb b9 0f 1f 80 00 00 00 00 8b 05 6a 2b 2c 00 48 63 d2 48 63 ff 85 c0 75 18 b8 2e 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 59 f3 c3 0f 1f 80 00 00 00 00 53 48 89 f3 48
[ 262.060708] RSP: 002b:00007ffdc5c4e2d8 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
[ 262.070112] RAX: ffffffffffffffda RBX: 000000005c73c202 RCX: 00007f28e91a8b87
[ 262.079087] RDX: 0000000000000000 RSI: 00007ffdc5c4e340 RDI: 0000000000000003
[ 262.088122] RBP: 0000000000000000 R08: 0000000000000001 R09: 000000000000000c
[ 262.097157] R10: 000000000000000c R11: 0000000000000246 R12: 0000000000000001
[ 262.106207] R13: 000000000067b4e0 R14: 00007ffdc5c5248c R15: 00007ffdc5c52480
[ 262.115271] Modules linked in: act_tunnel_key act_skbmod act_simple act_connmark nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 act_csum libcrc32c act_meta_skbtcindex act_meta_skbprio act_meta_mark act_ife ife act_police act_sample psample act_gact veth nfsv3 nfs_acl nfs lockd grace fscache bridge stp llc intel_rapl sb_edac mlx5_ib x86_pkg_temp_thermal sunrpc intel_powerclamp coretemp ib_uverbs kvm_intel ib_core kvm irqbypass mlx5_core crct10dif_pclmul crc32_pclmul crc32c_intel igb ghash_clmulni_intel intel_cstate mlxfw iTCO_wdt devlink intel_uncore iTCO_vendor_support ipmi_ssif ptp mei_me intel_rapl_perf ioatdma joydev pps_core ses mei i2c_i801 pcspkr enclosure lpc_ich dca wmi ipmi_si ipmi_devintf ipmi_msghandler acpi_pad acpi_power_meter pcc_cpufreq ast i2c_algo_bit drm_kms_helper ttm drm mpt3sas raid_class scsi_transport_sas
[ 262.204393] CR2: 00000000000000b0
[ 262.210390] ---[ end trace 2e41d786f2c7901a ]---
[ 262.226790] RIP: 0010:dst_cache_destroy+0x21/0xa0
[ 262.234083] Code: f4 ff ff ff eb f6 0f 1f 00 0f 1f 44 00 00 41 56 41 55 49 c7 c6 60 fe 35 af 41 54 55 49 89 fc 53 bd ff ff ff ff e8 ef 98 73 ff <49> 83 3c 24 00 75 35 eb 6c 4c 63 ed e8 de 98 73 ff 4a 8d 3c ed 40
[ 262.258311] RSP: 0018:ffff888316447160 EFLAGS: 00010282
[ 262.266304] RAX: 0000000000000000 RBX: ffff88835b3e2f00 RCX: ffffffffad1c5071
[ 262.276251] RDX: 0000000000000003 RSI: dffffc0000000000 RDI: 0000000000000297
[ 262.286208] RBP: 00000000ffffffff R08: fffffbfff5dd4e89 R09: fffffbfff5dd4e89
[ 262.296183] R10: 0000000000000001 R11: fffffbfff5dd4e88 R12: 00000000000000b0
[ 262.306157] R13: ffff8883267a10c0 R14: ffffffffaf35fe60 R15: 0000000000000001
[ 262.316139] FS: 00007f28ea3e6400(0000) GS:ffff888364200000(0000) knlGS:0000000000000000
[ 262.327146] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 262.335815] CR2: 00000000000000b0 CR3: 00000003178ae004 CR4: 00000000001606e0
Fixes: 41411e2fd6b8 ("net/sched: act_tunnel_key: Add dst_cache support")
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
Changes from V1 to V2:
- Extract metadata->dst error handler fix into standalone patch that targets
net.
net/sched/act_tunnel_key.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
index 3404af72b4c1..fc2b884b170c 100644
--- a/net/sched/act_tunnel_key.c
+++ b/net/sched/act_tunnel_key.c
@@ -201,8 +201,14 @@ static void tunnel_key_release_params(struct tcf_tunnel_key_params *p)
{
if (!p)
return;
- if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET)
+ if (p->tcft_action == TCA_TUNNEL_KEY_ACT_SET) {
+#ifdef CONFIG_DST_CACHE
+ struct ip_tunnel_info *info = &p->tcft_enc_metadata->u.tun_info;
+
+ dst_cache_destroy(&info->dst_cache);
+#endif
dst_release(&p->tcft_enc_metadata->dst);
+ }
kfree_rcu(p, rcu);
}
@@ -384,7 +390,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
release_dst_cache:
#ifdef CONFIG_DST_CACHE
- dst_cache_destroy(&metadata->u.tun_info.dst_cache);
+ if (metadata)
+ dst_cache_destroy(&metadata->u.tun_info.dst_cache);
#endif
release_tun_meta:
dst_release(&metadata->dst);
@@ -401,15 +408,8 @@ static void tunnel_key_release(struct tc_action *a)
{
struct tcf_tunnel_key *t = to_tunnel_key(a);
struct tcf_tunnel_key_params *params;
-#ifdef CONFIG_DST_CACHE
- struct ip_tunnel_info *info;
-#endif
params = rcu_dereference_protected(t->params, 1);
-#ifdef CONFIG_DST_CACHE
- info = ¶ms->tcft_enc_metadata->u.tun_info;
- dst_cache_destroy(&info->dst_cache);
-#endif
tunnel_key_release_params(params);
}
--
2.13.6
^ permalink raw reply related
* Re: [PATCH net] net: sched: act_tunnel_key: fix NULL pointer dereference during init
From: Davide Caratti @ 2019-02-25 15:36 UTC (permalink / raw)
To: Vlad Buslov, netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, wenxu, roid
In-Reply-To: <20190225152827.8741-1-vladbu@mellanox.com>
On Mon, 2019-02-25 at 17:28 +0200, Vlad Buslov wrote:
> Metadata pointer is only initialized for action TCA_TUNNEL_KEY_ACT_SET, but
> it is unconditionally dereferenced in tunnel_key_init() error handler.
> Verify that metadata pointer is not NULL before dereferencing it in
> tunnel_key_init error handling code.
>
> Fixes: ee28bb56ac5b ("net/sched: fix memory leak in act_tunnel_key_init()")
> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> ---
> net/sched/act_tunnel_key.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/sched/act_tunnel_key.c b/net/sched/act_tunnel_key.c
> index 8b43fe0130f7..3f943de9a2c9 100644
> --- a/net/sched/act_tunnel_key.c
> +++ b/net/sched/act_tunnel_key.c
> @@ -377,7 +377,8 @@ static int tunnel_key_init(struct net *net, struct nlattr *nla,
> return ret;
>
> release_tun_meta:
> - dst_release(&metadata->dst);
> + if (metadata)
> + dst_release(&metadata->dst);
>
> err_out:
> if (exists)
Reviewed-by: Davide Caratti <dcaratti@redhat.com>
^ 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