* Re: [PATCH] dt-bindings: net: dwmac: fix 'mac-mode' type
From: Jakub Kicinski @ 2019-09-21 18:46 UTC (permalink / raw)
To: Florian Fainelli
Cc: Alexandru Ardelean, netdev, devicetree, linux-kernel, davem,
robh+dt, peppe.cavallaro, alexandre.torgue, andrew
In-Reply-To: <f189cdbc-b399-7700-a39a-ba185df4af49@gmail.com>
On Fri, 20 Sep 2019 20:02:58 -0700, Florian Fainelli wrote:
> On 9/20/2019 6:11 PM, Jakub Kicinski wrote:
> > On Tue, 17 Sep 2019 13:30:52 +0300, Alexandru Ardelean wrote:
> >> The 'mac-mode' property is similar to 'phy-mode' and 'phy-connection-type',
> >> which are enums of mode strings.
> >>
> >> The 'dwmac' driver supports almost all modes declared in the 'phy-mode'
> >> enum (except for 1 or 2). But in general, there may be a case where
> >> 'mac-mode' becomes more generic and is moved as part of phylib or netdev.
> >>
> >> In any case, the 'mac-mode' field should be made an enum, and it also makes
> >> sense to just reference the 'phy-connection-type' from
> >> 'ethernet-controller.yaml'. That will also make it more future-proof for new
> >> modes.
> >>
> >> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> >
> > Applied, thank you!
> >
> > FWIW I had to add the Fixes tag by hand, either ozlabs patchwork or my
> > git-pw doesn't have the automagic handling there, yet.
>
> AFAICT the ozlabs patchwork instance does not do it, but if you have
> patchwork administrative rights (the jango administration panel I mean)
> then it is simple to add the regular expression to the list of tags that
> patchwork already recognized. Had tried getting that included by
> default, but it also counted all of those tags and therefore was not
> particularly fine grained:
>
> https://lists.ozlabs.org/pipermail/patchwork/2017-January/003910.html
Curious, it did seem to have counted the Fixes in the 'F' field on the
web UI but git-pw didn't pull it down 🤔
linux$ git checkout 92974a1d006ad8b30d53047c70974c9e065eb7df
Note: checking out '92974a1d006ad8b30d53047c70974c9e065eb7df'.
[...]
linux$ git pw patch apply 1163199 --signoff
11:41 linux$ git show
commit ac964661384b93ff3c9839c6d56f293195d54b4e (HEAD)
Author: Alexandru Ardelean <alexandru.ardelean@analog.com>
Date: Tue Sep 17 13:30:52 2019 +0300
dt-bindings: net: dwmac: fix 'mac-mode' type
The 'mac-mode' property is similar to 'phy-mode' and 'phy-connection-type',
which are enums of mode strings.
The 'dwmac' driver supports almost all modes declared in the 'phy-mode'
enum (except for 1 or 2). But in general, there may be a case where
'mac-mode' becomes more generic and is moved as part of phylib or netdev.
In any case, the 'mac-mode' field should be made an enum, and it also makes
sense to just reference the 'phy-connection-type' from
'ethernet-controller.yaml'. That will also make it more future-proof for new
modes.
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
11:41 linux$
^ permalink raw reply
* Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize
From: Jakub Kicinski @ 2019-09-21 19:30 UTC (permalink / raw)
To: Bjørn Mork; +Cc: netdev, linux-usb, Oliver Neukum
In-Reply-To: <87h855g68k.fsf@miraculix.mork.no>
On Sat, 21 Sep 2019 14:54:03 +0200, Bjørn Mork wrote:
> Jakub Kicinski <jakub.kicinski@netronome.com> writes:
> > On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote:
> >> Endpoints with zero wMaxPacketSize are not usable for transferring
> >> data. Ignore such endpoints when looking for valid in, out and
> >> status pipes, to make the drivers more robust against invalid and
> >> meaningless descriptors.
> >>
> >> The wMaxPacketSize of these endpoints are used for memory allocations
> >> and as divisors in many usbnet minidrivers. Avoiding zero is therefore
> >> critical.
> >>
> >> Signed-off-by: Bjørn Mork <bjorn@mork.no>
> >
> > Fixes tag would be useful. I'm not sure how far into stable we should
> > backport this.
>
> That would be commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), so I don't think
> a Fixes tag is very useful...
It's slightly useful to add it anyway, IMHO, even if it's 2.6.12,
because it may save people processing the patch checking how far
it applies. You already did the research, anyway.
Granted, that's a little `process-centric`, rather than `merit-centric`
view.
> I haven't verified how deep into the code you have been able to get with
> wMaxPacketSize being zero. But I don't think there ever has been much
> protection since it's so obviously "insane". There was no point in
> protecting against this as long as we considered the USB port a security
> barrier.
>
> I see that the v2.6.12-rc2 version of drivers/usb/net/usbnet.c (sic)
> already had this in it's genelink_tx_fixup():
<snip>
Thanks for the detailed analysis!
> So, to summarize: I believe the fix is valid for all stable versions.
>
> I'll leave it up to the more competent stable maintainers to decide how
> many, if any, it should be backported to. I will not cry if the answer
> is none.
Right, I'll put it in the stable queue, we'll see if it passes Dave's
and Greg's filters :)
> > Is this something that occurs on real devices or protection from
> > malicious ones?
>
> Only malicious ones AFAICS.
>
> I don't necessarily agree, but I believe the current policy makes this a
> "security" issue. CVEs have previously been allocated for similar
> crashes triggered by buggy USB descriptors. For some reason we are
> supposed to protect the system against *some* types of malicious
> hardware.
I see, the patch is fairly intrusive and very unlikely to cause to lead
to regressions on real devices, so regardless of the practical impact
shouldn't hurt.
> I am looking forward to the fixes coming up next to protect against
> malicious CPUs and microcode ;-)
I hope not before we retire..
Applied, queued for stable, thank you!
^ permalink raw reply
* Re: [PATCH net,stable] cdc_ncm: fix divide-by-zero caused by invalid wMaxPacketSize
From: Jakub Kicinski @ 2019-09-21 19:30 UTC (permalink / raw)
To: Bjørn Mork
Cc: netdev, linux-usb, Oliver Neukum, syzbot+ce366e2b8296e25d84f5
In-Reply-To: <20190918120147.4520-1-bjorn@mork.no>
On Wed, 18 Sep 2019 14:01:46 +0200, Bjørn Mork wrote:
> Endpoints with zero wMaxPacketSize are not usable for transferring
> data. Ignore such endpoints when looking for valid in, out and
> status pipes, to make the driver more robust against invalid and
> meaningless descriptors.
>
> The wMaxPacketSize of the out pipe is used as divisor. So this change
> fixes a divide-by-zero bug.
>
> Reported-by: syzbot+ce366e2b8296e25d84f5@syzkaller.appspotmail.com
> Signed-off-by: Bjørn Mork <bjorn@mork.no>
Applied, queued, thank you!
^ permalink raw reply
* Re: [PATCH] net: dsa: b53: Use the correct style for SPDX License Identifier
From: Florian Fainelli @ 2019-09-21 19:39 UTC (permalink / raw)
To: Nishad Kamdar, Andrew Lunn, Vivien Didelot, David S. Miller,
Greg Kroah-Hartman, Joe Perches, Uwe Kleine-König
Cc: netdev, linux-kernel
In-Reply-To: <20190921133011.GA2994@nishad>
On 9/21/2019 6:30 AM, Nishad Kamdar wrote:
> This patch corrects the SPDX License Identifier style
> in header file for Broadcom BCM53xx managed switch driver.
> For C header files Documentation/process/license-rules.rst
> mandates C-like comments (opposed to C source files where
> C++ style should be used)
>
> Changes made by using a script provided by Joe Perches here:
> https://lkml.org/lkml/2019/2/7/46.
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH] net: dsa: Use the correct style for SPDX License Identifier
From: Florian Fainelli @ 2019-09-21 19:39 UTC (permalink / raw)
To: Nishad Kamdar, Hauke Mehrtens, Andrew Lunn, Vivien Didelot,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Uwe Kleine-König
Cc: netdev, linux-kernel
In-Reply-To: <20190921134522.GA3575@nishad>
On 9/21/2019 6:45 AM, Nishad Kamdar wrote:
> This patch corrects the SPDX License Identifier style
> in header file for Distributed Switch Architecture drivers.
> For C header files Documentation/process/license-rules.rst
> mandates C-like comments (opposed to C source files where
> C++ style should be used)
>
> Changes made by using a script provided by Joe Perches here:
> https://lkml.org/lkml/2019/2/7/46.
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
--
Florian
^ permalink raw reply
* Re: [PATCH net] net: openvswitch: fix possible memleak on create vport fails
From: Jakub Kicinski @ 2019-09-22 1:06 UTC (permalink / raw)
To: xiangxia.m.yue; +Cc: gvrose8192, pshelar, netdev, Taehee Yoo
In-Reply-To: <1568734808-42628-1-git-send-email-xiangxia.m.yue@gmail.com>
On Tue, 17 Sep 2019 23:40:08 +0800, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> If we register a net device which name is not valid
> (dev_get_valid_name), register_netdevice will return err
> codes and will not run dev->priv_destructor. The memory
> will leak. This patch adds check in ovs_vport_free and
> set the vport NULL.
>
> Fixes: 309b66970ee2 ("net: openvswitch: do not free vport if register_netdevice() is failed.")
> Cc: Taehee Yoo <ap420073@gmail.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Thanks for the patch, I see what you're trying to do, but..
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index d2437b5..074c43f 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> @@ -159,7 +159,6 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
> struct internal_dev *internal_dev;
> struct net_device *dev;
> int err;
> - bool free_vport = true;
>
> vport = ovs_vport_alloc(0, &ovs_internal_vport_ops, parms);
> if (IS_ERR(vport)) {
> @@ -190,10 +189,8 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
>
> rtnl_lock();
> err = register_netdevice(vport->dev);
> - if (err) {
> - free_vport = false;
> + if (err)
> goto error_unlock;
> - }
>
> dev_set_promiscuity(vport->dev, 1);
> rtnl_unlock();
> @@ -207,8 +204,7 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
> error_free_netdev:
> free_netdev(dev);
> error_free_vport:
> - if (free_vport)
> - ovs_vport_free(vport);
> + ovs_vport_free(vport);
> error:
> return ERR_PTR(err);
> }
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index 3fc38d1..281259a 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -157,11 +157,20 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops,
> */
> void ovs_vport_free(struct vport *vport)
> {
> + /* We should check whether vport is NULL.
> + * We may free it again, for example in internal_dev_create
> + * if register_netdevice fails, vport may have been freed via
> + * internal_dev_destructor.
> + */
> + if (unlikely(!vport))
> + return;
> +
> /* vport is freed from RCU callback or error path, Therefore
> * it is safe to use raw dereference.
> */
> kfree(rcu_dereference_raw(vport->upcall_portids));
> kfree(vport);
> + vport = NULL;
vport here is a function argument, seems like setting it to NULL
right before the function ends will do nothing. Should we rather
set internal_dev->vport to NULL somehow?
Perhaps someone more familiar with OvS can chime in and review..
> }
> EXPORT_SYMBOL_GPL(ovs_vport_free);
>
^ permalink raw reply
* Re: [PATCH net v3] net/phy: fix DP83865 10 Mbps HDX loopback disable function
From: Jakub Kicinski @ 2019-09-22 1:29 UTC (permalink / raw)
To: Peter Mamonov, andrew
Cc: Florian Fainelli, Heiner Kallweit, David S. Miller, netdev,
linux-kernel
In-Reply-To: <20190918162755.24024-1-pmamonov@gmail.com>
On Wed, 18 Sep 2019 19:27:55 +0300, Peter Mamonov wrote:
> According to the DP83865 datasheet "the 10 Mbps HDX loopback can be
> disabled in the expanded memory register 0x1C0.1". The driver erroneously
> used bit 0 instead of bit 1.
>
> Fixes: 4621bf129856 ("phy: Add file missed in previous commit.")
> Signed-off-by: Peter Mamonov <pmamonov@gmail.com>
Applied, queued, thank you!
^ permalink raw reply
* Re: [PATCH] drivers/net/fjes: fix a potential NULL pointer dereference
From: Jakub Kicinski @ 2019-09-22 1:40 UTC (permalink / raw)
To: Allen Pais; +Cc: netdev, davem, linux-kernel
In-Reply-To: <1568824395-4162-1-git-send-email-allen.pais@oracle.com>
On Wed, 18 Sep 2019 22:03:15 +0530, Allen Pais wrote:
> alloc_workqueue is not checked for errors and as a result,
> a potential NULL dereference could occur.
>
> Signed-off-by: Allen Pais <allen.pais@oracle.com>
If I'm looking at this right you are jumping to err_free_netdev without
setting the err variable. It must had been set to 0 from the return of
fjes_sw_init(). This means we will free the netdev, and return 0. This
means probe will not fail and driver's remove function will be run
at some point. fjes_remove it will try to free the netdev again.
Looks like there's another existing bug here in that the work queues
are not free when something fails in fjes_probe, just the netdev.
Once you untangle that, and before you post a v2, could you please try
to identify which commit introduced the regression and provide an
appropriate "Fixes" tag?
> diff --git a/drivers/net/fjes/fjes_main.c b/drivers/net/fjes/fjes_main.c
> index bbbc1dc..2d04104 100644
> --- a/drivers/net/fjes/fjes_main.c
> +++ b/drivers/net/fjes/fjes_main.c
> @@ -1237,8 +1237,15 @@ static int fjes_probe(struct platform_device *plat_dev)
> adapter->open_guard = false;
>
> adapter->txrx_wq = alloc_workqueue(DRV_NAME "/txrx", WQ_MEM_RECLAIM, 0);
> + if (unlikely(!adapter->txrx_wq))
> + goto err_free_netdev;
> +
> adapter->control_wq = alloc_workqueue(DRV_NAME "/control",
> WQ_MEM_RECLAIM, 0);
> + if (unlikely(!adapter->control_wq)) {
> + destroy_workqueue(adapter->txrx_wq);
> + goto err_free_netdev;
> + }
>
> INIT_WORK(&adapter->tx_stall_task, fjes_tx_stall_task);
> INIT_WORK(&adapter->raise_intr_rxdata_task,
^ permalink raw reply
* Re: [PATCH] net: qrtr: Stop rx_worker before freeing node
From: Jakub Kicinski @ 2019-09-22 1:54 UTC (permalink / raw)
To: Bjorn Andersson
Cc: David S. Miller, netdev, linux-kernel, linux-arm-msm,
Srinivas Kandagatla, stable
In-Reply-To: <20190918172117.4116-1-bjorn.andersson@linaro.org>
On Wed, 18 Sep 2019 10:21:17 -0700, Bjorn Andersson wrote:
> As the endpoint is unregistered there might still be work pending to
> handle incoming messages, which will result in a use after free
> scenario. The plan is to remove the rx_worker, but until then (and for
> stable@) ensure that the work is stopped before the node is freed.
>
> Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
> Cc: stable@vger.kernel.org
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Thanks! Applied, and queued for 4.9+.
FWIW Dave handles the stable submission himself, so no need to CC
stable on networking patches:
https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html#q-i-see-a-network-patch-and-i-think-it-should-be-backported-to-stable
^ permalink raw reply
* Re: [PATCH net] net: sched: fix possible crash in tcf_action_destroy()
From: Jakub Kicinski @ 2019-09-22 2:08 UTC (permalink / raw)
To: Cong Wang, Eric Dumazet
Cc: Eric Dumazet, David S . Miller, netdev, syzbot, Vlad Buslov,
Jiri Pirko
In-Reply-To: <CAM_iQpVyJDeScQDL6vHNAN9gu5a3c0forQ2Ko7eQihawRO_Sdw@mail.gmail.com>
On Wed, 18 Sep 2019 14:37:21 -0700, Cong Wang wrote:
> On Wed, Sep 18, 2019 at 12:57 PM 'Eric Dumazet' via syzkaller
> <syzkaller@googlegroups.com> wrote:
> >
> > If the allocation done in tcf_exts_init() failed,
> > we end up with a NULL pointer in exts->actions.
> ...
> > diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> > index efd3cfb80a2ad775dc8ab3c4900bd73d52c7aaad..9aef93300f1c11791acbb9262dfe77996872eafe 100644
> > --- a/net/sched/cls_api.c
> > +++ b/net/sched/cls_api.c
> > @@ -3027,8 +3027,10 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
> > void tcf_exts_destroy(struct tcf_exts *exts)
> > {
> > #ifdef CONFIG_NET_CLS_ACT
> > - tcf_action_destroy(exts->actions, TCA_ACT_UNBIND);
> > - kfree(exts->actions);
> > + if (exts->actions) {
>
> I think it is _slightly_ better to check exts->nr_actions!=0 here,
> as it would help exts->actions!=NULL&& exts->nr_actions==0
> cases too.
>
> What do you think?
Alternatively, since tcf_exts_destroy() now takes NULL, and so
obviously does kfree() - perhaps tcf_action_destroy() should
return early if actions are NULL?
^ permalink raw reply
* Re: [PATCH] net: remove netx ethernet driver
From: Jakub Kicinski @ 2019-09-22 2:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David S. Miller, Uwe Kleine-König, Sascha Hauer,
linux-kernel, netdev
In-Reply-To: <20190918202243.2880006-1-arnd@arndb.de>
On Wed, 18 Sep 2019 22:21:24 +0200, Arnd Bergmann wrote:
> The ARM netx platform got removed in 5.3, so this driver
> is now useless.
>
> Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thank you!
FWIW there seems to be a lone mention of something called netx_timer in
linux-next, not sure if that's related too..
linux $ git grep netx_timer
Documentation/admin-guide/kernel-parameters.txt: [ARM] imx_timer1,OSTS,netx_timer,mpu_timer2,
^ permalink raw reply
* Re: [Patch net] net_sched: add max len check for TCA_KIND
From: Jakub Kicinski @ 2019-09-22 2:24 UTC (permalink / raw)
To: Cong Wang
Cc: David Ahern, Linux Kernel Network Developers, syzbot,
Jamal Hadi Salim, Jiri Pirko
In-Reply-To: <CAM_iQpXa=Kru2tXKwrErM9VsO40coBf9gKLRfwC3e8owKZG+0w@mail.gmail.com>
On Wed, 18 Sep 2019 22:15:24 -0700, Cong Wang wrote:
> On Wed, Sep 18, 2019 at 7:41 PM David Ahern <dsahern@gmail.com> wrote:
> > On 9/18/19 5:24 PM, Cong Wang wrote:
> > > The TCA_KIND attribute is of NLA_STRING which does not check
> > > the NUL char. KMSAN reported an uninit-value of TCA_KIND which
> > > is likely caused by the lack of NUL.
> > >
> > > Change it to NLA_NUL_STRING and add a max len too.
> > >
> > > Fixes: 8b4c3cdd9dd8 ("net: sched: Add policy validation for tc attributes")
> >
> > The commit referenced here did not introduce the ability to go beyond
> > memory boundaries with string comparisons. Rather, it was not complete
> > solution for attribute validation. I say that wrt to the fix getting
> > propagated to the correct stable releases.
>
> I think this patch should be backported to wherever commit 8b4c3cdd9dd8
> goes, this is why I picked it as Fixes.
Applied, queued for 4.14+, thanks!
> >
> > > Reported-and-tested-by: syzbot+618aacd49e8c8b8486bd@syzkaller.appspotmail.com
> >
> > What is the actual sysbot report?
>
> https://marc.info/?l=linux-kernel&m=156862916112881&w=2
^ permalink raw reply
* Re: [Patch net] net_sched: add policy validation for action attributes
From: Jakub Kicinski @ 2019-09-22 2:38 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, David Ahern, Jamal Hadi Salim, Jiri Pirko
In-Reply-To: <20190919014443.32581-1-xiyou.wangcong@gmail.com>
On Wed, 18 Sep 2019 18:44:43 -0700, Cong Wang wrote:
> Similar to commit 8b4c3cdd9dd8
> ("net: sched: Add policy validation for tc attributes"), we need
> to add proper policy validation for TC action attributes too.
>
> Cc: David Ahern <dsahern@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Applied, thank you!
Queued for stable 5.2+ let me know if we should backport to long term
kernels, or not at all.
^ permalink raw reply
* Re: [PATCH net] net: dsa: sja1105: Add dependency for NET_DSA_SJA1105_TAS
From: Jakub Kicinski @ 2019-09-22 2:43 UTC (permalink / raw)
To: Mao Wenan
Cc: olteanv, andrew, vivien.didelot, f.fainelli, davem, linux-kernel,
netdev, kernel-janitors
In-Reply-To: <20190919063819.164826-1-maowenan@huawei.com>
On Thu, 19 Sep 2019 14:38:19 +0800, Mao Wenan wrote:
> If CONFIG_NET_DSA_SJA1105_TAS=y and CONFIG_NET_SCH_TAPRIO=n,
> below error can be found:
> drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_setup_tc_taprio':
> sja1105_tas.c:(.text+0x318): undefined reference to `taprio_offload_free'
> sja1105_tas.c:(.text+0x590): undefined reference to `taprio_offload_get'
> drivers/net/dsa/sja1105/sja1105_tas.o: In function `sja1105_tas_teardown':
> sja1105_tas.c:(.text+0x610): undefined reference to `taprio_offload_free'
> make: *** [vmlinux] Error 1
>
> sja1105_tas needs tc-taprio, so this patch add the dependency for it.
>
> Fixes: 317ab5b86c8e ("net: dsa: sja1105: Configure the Time-Aware Scheduler via tc-taprio offload")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH] [v2] net: stmmac: selftest: avoid large stack usage
From: Jakub Kicinski @ 2019-09-22 2:49 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu, David S. Miller,
Maxime Coquelin, netdev, linux-stm32, linux-arm-kernel,
linux-kernel
In-Reply-To: <20190919123416.3070938-1-arnd@arndb.de>
On Thu, 19 Sep 2019 14:33:43 +0200, Arnd Bergmann wrote:
> Putting a struct stmmac_rss object on the stack is a bad idea,
> as it exceeds the warning limit for a stack frame on 32-bit architectures:
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:1221:12: error: stack frame size of 1208 bytes in function '__stmmac_test_l3filt' [-Werror,-Wframe-larger-than=]
> drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c:1338:12: error: stack frame size of 1208 bytes in function '__stmmac_test_l4filt' [-Werror,-Wframe-larger-than=]
>
> As the object is the trivial empty case, change the called function
> to accept a NULL pointer to mean the same thing and remove the
> large variable in the two callers.
>
> Fixes: 4647e021193d ("net: stmmac: selftests: Add selftest for L3/L4 Filters")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH] dimlib: make DIMLIB a hidden symbol
From: Jakub Kicinski @ 2019-09-22 3:06 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: Tal Gilboa, Saeed Mahameed, linux-kernel, netdev
In-Reply-To: <20190920133115.12802-1-uwe@kleine-koenig.org>
On Fri, 20 Sep 2019 15:31:15 +0200, Uwe Kleine-König wrote:
> According to Tal Gilboa the only benefit from DIM comes from a driver
> that uses it. So it doesn't make sense to make this symbol user visible,
> instead all drivers that use it should select it (as is already the case
> AFAICT).
>
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> lib/Kconfig | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/Kconfig b/lib/Kconfig
> index cc04124ed8f7..9fe8a21fd183 100644
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -555,8 +555,7 @@ config SIGNATURE
> Implementation is done using GnuPG MPI library
>
> config DIMLIB
> - bool "DIM library"
> - default y
> + bool
> help
> Dynamic Interrupt Moderation library.
> Implements an algorithm for dynamically change CQ moderation values
Hi Uwe! Looks like in the net tree there is a spelling mistake and
moderation is spelled "modertion":
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/lib/Kconfig#n562
I'm not seeing any patch to fix that anywhere, is it possible you have
some local change in your tree?
^ permalink raw reply
* Re: [PATCH net] net: ena: Add dependency for ENA_ETHERNET
From: Jakub Kicinski @ 2019-09-22 3:07 UTC (permalink / raw)
To: Mao Wenan
Cc: netanel, saeedb, zorik, davem, netdev, linux-kernel,
kernel-janitors, Uwe Kleine-König
In-Reply-To: <20190920084405.140750-1-maowenan@huawei.com>
On Fri, 20 Sep 2019 16:44:05 +0800, Mao Wenan wrote:
> If CONFIG_ENA_ETHERNET=y and CONFIG_DIMLIB=n,
> below erros can be found:
> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_dim_work':
> ena_netdev.c:(.text+0x21cc): undefined reference to `net_dim_get_rx_moderation'
> ena_netdev.c:(.text+0x21cc): relocation truncated to
> fit: R_AARCH64_CALL26 against undefined symbol `net_dim_get_rx_moderation'
> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_io_poll':
> ena_netdev.c:(.text+0x7bd4): undefined reference to `net_dim'
> ena_netdev.c:(.text+0x7bd4): relocation truncated to fit:
> R_AARCH64_CALL26 against undefined symbol `net_dim'
>
> After commit 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive
> interrupt moderation"), it introduces dim algorithm, which configured by CONFIG_DIMLIB.
>
> Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
Thank you Mao, shortly after you posted your patch Uwe proposed to make
DIMLIB a hidden symbol:
https://lore.kernel.org/netdev/a85be675-ce56-f7ba-29e9-b749073aab6c@kleine-koenig.org/T/#t
That patch will likely be applied soon, could you please rework your
patch to use the "select" keyword instead of "depends". That's what
other users do.
> diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig
> index 69ca99d..fe46df4 100644
> --- a/drivers/net/ethernet/amazon/Kconfig
> +++ b/drivers/net/ethernet/amazon/Kconfig
> @@ -18,7 +18,7 @@ if NET_VENDOR_AMAZON
>
> config ENA_ETHERNET
> tristate "Elastic Network Adapter (ENA) support"
> - depends on PCI_MSI && !CPU_BIG_ENDIAN
> + depends on PCI_MSI && !CPU_BIG_ENDIAN && DIMLIB
> ---help---
> This driver supports Elastic Network Adapter (ENA)"
>
^ permalink raw reply
* Re: [PATCH net] net: openvswitch: fix possible memleak on create vport fails
From: Pravin Shelar @ 2019-09-22 3:17 UTC (permalink / raw)
To: Tonghao Zhang, Hillf Danton
Cc: Greg Rose, Linux Kernel Network Developers, Taehee Yoo
In-Reply-To: <1568734808-42628-1-git-send-email-xiangxia.m.yue@gmail.com>
On Sat, Sep 21, 2019 at 8:07 AM <xiangxia.m.yue@gmail.com> wrote:
>
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> If we register a net device which name is not valid
> (dev_get_valid_name), register_netdevice will return err
> codes and will not run dev->priv_destructor. The memory
> will leak. This patch adds check in ovs_vport_free and
> set the vport NULL.
>
> Fixes: 309b66970ee2 ("net: openvswitch: do not free vport if register_netdevice() is failed.")
> Cc: Taehee Yoo <ap420073@gmail.com>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> net/openvswitch/vport-internal_dev.c | 8 ++------
> net/openvswitch/vport.c | 9 +++++++++
> 2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/net/openvswitch/vport-internal_dev.c b/net/openvswitch/vport-internal_dev.c
> index d2437b5..074c43f 100644
> --- a/net/openvswitch/vport-internal_dev.c
> +++ b/net/openvswitch/vport-internal_dev.c
> @@ -159,7 +159,6 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
> struct internal_dev *internal_dev;
> struct net_device *dev;
> int err;
> - bool free_vport = true;
>
> vport = ovs_vport_alloc(0, &ovs_internal_vport_ops, parms);
> if (IS_ERR(vport)) {
> @@ -190,10 +189,8 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
>
> rtnl_lock();
> err = register_netdevice(vport->dev);
> - if (err) {
> - free_vport = false;
> + if (err)
> goto error_unlock;
> - }
>
> dev_set_promiscuity(vport->dev, 1);
> rtnl_unlock();
> @@ -207,8 +204,7 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
> error_free_netdev:
> free_netdev(dev);
> error_free_vport:
> - if (free_vport)
> - ovs_vport_free(vport);
> + ovs_vport_free(vport);
> error:
> return ERR_PTR(err);
> }
> diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
> index 3fc38d1..281259a 100644
> --- a/net/openvswitch/vport.c
> +++ b/net/openvswitch/vport.c
> @@ -157,11 +157,20 @@ struct vport *ovs_vport_alloc(int priv_size, const struct vport_ops *ops,
> */
> void ovs_vport_free(struct vport *vport)
> {
> + /* We should check whether vport is NULL.
> + * We may free it again, for example in internal_dev_create
> + * if register_netdevice fails, vport may have been freed via
> + * internal_dev_destructor.
> + */
> + if (unlikely(!vport))
> + return;
> +
> /* vport is freed from RCU callback or error path, Therefore
> * it is safe to use raw dereference.
> */
> kfree(rcu_dereference_raw(vport->upcall_portids));
> kfree(vport);
> + vport = NULL;
> }
> EXPORT_SYMBOL_GPL(ovs_vport_free);
>
> --
There is already patch a patch to fix this memory leak.
https://patchwork.ozlabs.org/patch/1144316/
Can you or Hillf post it on netdev list?
Thanks.
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Pravin Shelar @ 2019-09-22 3:18 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jakub Kicinski, Vlad Buslov, David Miller, Paul Blakey,
netdev@vger.kernel.org, Jiri Pirko, Cong Wang, Jamal Hadi Salim,
Simon Horman, Alexei Starovoitov, Or Gerlitz
In-Reply-To: <f1983a74-d144-6d21-9b20-59cea9afc366@iogearbox.net>
On Fri, Sep 20, 2019 at 5:06 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 9/21/19 12:56 AM, Jakub Kicinski wrote:
> [...]
> >> I thought idea of stuffing things into skb extensions are only justified if
> >> it's not enabled by default for everyone. :(
> >>
> >> [0] https://lore.kernel.org/netdev/CAHC9VhSz1_KA1tCJtNjwK26BOkGhKGbPT7v1O82mWPduvWwd4A@mail.gmail.com/T/#u
> >
> > The skb ext allocation is only done with GOTO_CHAIN, which AFAIK only
> > has practical use for offload. We could perhaps add another static
> > branch there or move the OvS static branch out of the OvS module so
> > there are no linking issues?
> >
> > I personally have little sympathy for this piece of code, it is perhaps
> > the purest form of a wobbly narrow-use construct pushed into TC for HW
> > offload.
> >
> > Any suggestions on the way forward? :(
>
> Presumably there are no clean solutions here, but on the top of my head for
> this use case, you'd need to /own/ the underlying datapath anyway, so couldn't
> you program the OVS key->recirc_id based on skb->mark (or alternatively via
> skb->tc_index) which was previously set by tc ingress?
>
If we are going down this path, tc_index should be used. skb->mark is
already used in OVS flow match, overloading it for this purpose would
be complicated.
^ permalink raw reply
* Re: [PATCH] qede: qede_fp: simplify a bit 'qede_rx_build_skb()'
From: Jakub Kicinski @ 2019-09-22 3:18 UTC (permalink / raw)
To: Christophe JAILLET
Cc: aelior, GR-everest-linux-l2, davem, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <20190920045656.3725-1-christophe.jaillet@wanadoo.fr>
On Fri, 20 Sep 2019 06:56:56 +0200, Christophe JAILLET wrote:
> Use 'skb_put_data()' instead of rewritting it.
> This improves readability.
>
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH V2 net 1/1] net/tls(TLS_SW): Fix list_del double free caused by a race condition in tls_tx_records
From: Pooja Trivedi @ 2019-09-22 3:19 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netdev, davem, daniel, john.fastabend, davejwatson, aviadye,
borisp, Pooja Trivedi, Mallesham Jatharakonda
In-Reply-To: <20190918144528.57a5cb50@cakuba.netronome.com>
On Wed, Sep 18, 2019 at 5:45 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 18 Sep 2019 17:37:44 -0400, Pooja Trivedi wrote:
> > Hi Jakub,
> >
> > I have explained one potential way for the race to happen in my
> > original message to the netdev mailing list here:
> > https://marc.info/?l=linux-netdev&m=156805120229554&w=2
> >
> > Here is the part out of there that's relevant to your question:
> >
> > -----------------------------------------
> >
> > One potential way for race condition to appear:
> >
> > When under tcp memory pressure, Thread 1 takes the following code path:
> > do_sendfile ---> ... ---> .... ---> tls_sw_sendpage --->
> > tls_sw_do_sendpage ---> tls_tx_records ---> tls_push_sg --->
> > do_tcp_sendpages ---> sk_stream_wait_memory ---> sk_wait_event
>
> Ugh, so do_tcp_sendpages() can also release the lock :/
>
> Since the problem occurs in tls_sw_do_sendpage() and
> tls_sw_do_sendmsg() as well, should we perhaps fix it at that level?
That won't do because tls_tx_records also gets called when completion
callbacks schedule delayed work. That was the code path that caused
the crash for my test. Cavium's nitrox crypto offload driver calling
tls_encrypt_done, which calls schedule_delayed_work. Delayed work that
was scheduled would then be processed by tx_work_handler.
Notice in my previous reply,
"Thread 2 code path:
tx_work_handler ---> tls_tx_records"
"Thread 2 code path:
tx_work_handler ---> tls_tx_records"
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Alexei Starovoitov @ 2019-09-22 4:51 UTC (permalink / raw)
To: Pravin Shelar
Cc: Daniel Borkmann, Jakub Kicinski, Vlad Buslov, David Miller,
Paul Blakey, netdev@vger.kernel.org, Jiri Pirko, Cong Wang,
Jamal Hadi Salim, Simon Horman, Or Gerlitz
In-Reply-To: <CAOrHB_Bqhq6cy6QgyEymHaUDk-BN9fkkQ-rzCqWeN35sqiym4w@mail.gmail.com>
On Sat, Sep 21, 2019 at 8:15 PM Pravin Shelar <pshelar@ovn.org> wrote:
> > > Any suggestions on the way forward? :(
since there is no clear and agreed by everyone path forward the simple
revert is the best option.
Next release cycle you can come up with something that works for everyone.
^ permalink raw reply
* Re: [PATCH net] net: ena: Add dependency for ENA_ETHERNET
From: maowenan @ 2019-09-22 5:08 UTC (permalink / raw)
To: Jakub Kicinski
Cc: netanel, saeedb, zorik, davem, netdev, linux-kernel,
kernel-janitors, Uwe Kleine-König
In-Reply-To: <20190921200741.1c3289e8@cakuba.netronome.com>
On 2019/9/22 11:07, Jakub Kicinski wrote:
> On Fri, 20 Sep 2019 16:44:05 +0800, Mao Wenan wrote:
>> If CONFIG_ENA_ETHERNET=y and CONFIG_DIMLIB=n,
>> below erros can be found:
>> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_dim_work':
>> ena_netdev.c:(.text+0x21cc): undefined reference to `net_dim_get_rx_moderation'
>> ena_netdev.c:(.text+0x21cc): relocation truncated to
>> fit: R_AARCH64_CALL26 against undefined symbol `net_dim_get_rx_moderation'
>> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_io_poll':
>> ena_netdev.c:(.text+0x7bd4): undefined reference to `net_dim'
>> ena_netdev.c:(.text+0x7bd4): relocation truncated to fit:
>> R_AARCH64_CALL26 against undefined symbol `net_dim'
>>
>> After commit 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive
>> interrupt moderation"), it introduces dim algorithm, which configured by CONFIG_DIMLIB.
>>
>> Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
>> Signed-off-by: Mao Wenan <maowenan@huawei.com>
>
> Thank you Mao, shortly after you posted your patch Uwe proposed to make
> DIMLIB a hidden symbol:
>
> https://lore.kernel.org/netdev/a85be675-ce56-f7ba-29e9-b749073aab6c@kleine-koenig.org/T/#t
>
> That patch will likely be applied soon, could you please rework your
> patch to use the "select" keyword instead of "depends". That's what
> other users do.
Ok, I will send v2.
>
>> diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig
>> index 69ca99d..fe46df4 100644
>> --- a/drivers/net/ethernet/amazon/Kconfig
>> +++ b/drivers/net/ethernet/amazon/Kconfig
>> @@ -18,7 +18,7 @@ if NET_VENDOR_AMAZON
>>
>> config ENA_ETHERNET
>> tristate "Elastic Network Adapter (ENA) support"
>> - depends on PCI_MSI && !CPU_BIG_ENDIAN
>> + depends on PCI_MSI && !CPU_BIG_ENDIAN && DIMLIB
>> ---help---
>> This driver supports Elastic Network Adapter (ENA)"
>>
>
>
> .
>
^ permalink raw reply
* [PATCH v2 net] net: ena: Select DIMLIB for ENA_ETHERNET
From: Mao Wenan @ 2019-09-22 5:38 UTC (permalink / raw)
To: netanel, saeedb, zorik, davem
Cc: netdev, linux-kernel, kernel-janitors, Mao Wenan
In-Reply-To: <20190921200741.1c3289e8@cakuba.netronome.com>
If CONFIG_ENA_ETHERNET=y and CONFIG_DIMLIB=n,
below erros can be found:
drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_dim_work':
ena_netdev.c:(.text+0x21cc): undefined reference to `net_dim_get_rx_moderation'
ena_netdev.c:(.text+0x21cc): relocation truncated to
fit: R_AARCH64_CALL26 against undefined symbol `net_dim_get_rx_moderation'
drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_io_poll':
ena_netdev.c:(.text+0x7bd4): undefined reference to `net_dim'
ena_netdev.c:(.text+0x7bd4): relocation truncated to fit:
R_AARCH64_CALL26 against undefined symbol `net_dim'
After commit 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive
interrupt moderation"), it introduces dim algorithm, which configured by CONFIG_DIMLIB.
So, this patch is to select DIMLIB for ENA_ETHERNET.
Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
v2: change subject of patch, use the "select" keyword instead of "depends".
drivers/net/ethernet/amazon/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/amazon/Kconfig b/drivers/net/ethernet/amazon/Kconfig
index 69ca99d8ac26..cca72a75f551 100644
--- a/drivers/net/ethernet/amazon/Kconfig
+++ b/drivers/net/ethernet/amazon/Kconfig
@@ -19,6 +19,7 @@ if NET_VENDOR_AMAZON
config ENA_ETHERNET
tristate "Elastic Network Adapter (ENA) support"
depends on PCI_MSI && !CPU_BIG_ENDIAN
+ select DIMLIB
---help---
This driver supports Elastic Network Adapter (ENA)"
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net] net: openvswitch: fix possible memleak on createvport fails
From: Pravin Shelar @ 2019-09-22 5:50 UTC (permalink / raw)
To: Hillf Danton
Cc: Tonghao Zhang, Greg Rose, Linux Kernel Network Developers,
Taehee Yoo
In-Reply-To: <5d86ef21.1c69fb81.d4519.2861SMTPIN_ADDED_MISSING@mx.google.com>
On Sat, Sep 21, 2019 at 8:48 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Sun, 9 Sep 2019 11:14 from Pravin Shelar <pshelar@ovn.org>
>
> >
>
> > There is already patch a patch to fix this memory leak.
>
> > https://patchwork.ozlabs.org/patch/1144316/
>
> > Can you or Hillf post it on netdev list?
>
>
>
> Was that posted without netdev Cced?
I do not see your patch on netdev patchwork, repost of the patch would
put it on netdev patchwork.
^ 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