* Re: [PATCH v3 net-next 01/11] net: dsa: microchip: Replace license with GPL
From: Pavel Machek @ 2018-10-24 11:05 UTC (permalink / raw)
To: Tristram.Ha
Cc: Andrew Lunn, Florian Fainelli, Ruediger Schmitt, UNGLinuxDriver,
netdev
In-Reply-To: <1540261575-1889-2-git-send-email-Tristram.Ha@microchip.com>
[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]
On Mon 2018-10-22 19:26:05, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Replace license with GPL.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Acked-by: Pavel Machek <pavel@ucw.cz>
> @@ -1,19 +1,20 @@
> /*
> * Microchip KSZ9477 register definitions
> *
> - * Copyright (C) 2017
> + * Copyright (C) 2017-2018 Microchip Technology Inc.
> *
> - * Permission to use, copy, modify, and/or distribute this software for any
> - * purpose with or without fee is hereby granted, provided that the above
> - * copyright notice and this permission notice appear in all copies.
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version 2
> + * of the License, or (at your option) any later version.
> *
But going to SPDX and getting rid of this legaleese would be even
better.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH bpf] bpf: devmap: fix wrong interface selection in notifier_call
From: Taehee Yoo @ 2018-10-24 11:03 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: ast, Netdev, john.fastabend
In-Reply-To: <82dbe1ae-bf7b-2eca-1d19-b6c72c5a79c8@iogearbox.net>
Hi Daniel!
On Wed, 24 Oct 2018 at 18:07, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> [ +John ]
>
> On 10/22/2018 05:41 PM, Taehee Yoo wrote:
> > The dev_map_notification() removes interface in devmap if
> > unregistering interface's ifindex is same.
> > But only checking ifindex is not enough because other netns can have
> > same ifindex. so that wrong interface selection could occurred.
> > Hence the net_eq() is needed.
> >
> > Fixes: 2ddf71e23cc2 ("net: add notifier hooks for devmap bpf map")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> > kernel/bpf/devmap.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> > index 141710b82a6c..0d9211e49a4a 100644
> > --- a/kernel/bpf/devmap.c
> > +++ b/kernel/bpf/devmap.c
> > @@ -496,6 +496,7 @@ static int dev_map_notification(struct notifier_block *notifier,
> > ulong event, void *ptr)
> > {
> > struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
> > + struct net *net = dev_net(netdev);
> > struct bpf_dtab *dtab;
> > int i;
> >
> > @@ -512,7 +513,7 @@ static int dev_map_notification(struct notifier_block *notifier,
> > struct bpf_dtab_netdev *dev, *odev;
> >
> > dev = READ_ONCE(dtab->netdev_map[i]);
> > - if (!dev ||
> > + if (!dev || !net_eq(net, dev_net(dev->dev)) ||
> > dev->dev->ifindex != netdev->ifindex)
>
> Can't we just compare netdev pointers directly here?
>
Thank you for review!
I tested what you suggested, It works well and it's more simple.
So that I will send v2 patch that just compares netdev pointer instead of
using ifindex and net_eq().
> > continue;
> > odev = cmpxchg(&dtab->netdev_map[i], dev, NULL);
> >
>
Thanks!
^ permalink raw reply
* Re: [RFC PATCH v2 00/10] udp: implement GRO support
From: Steffen Klassert @ 2018-10-24 10:55 UTC (permalink / raw)
To: Paolo Abeni; +Cc: netdev, Willem de Bruijn
In-Reply-To: <7aecb7871f3e330debdeb81fd8a401ec5692fa81.camel@redhat.com>
On Tue, Oct 23, 2018 at 02:22:12PM +0200, Paolo Abeni wrote:
> On Tue, 2018-10-23 at 14:10 +0200, Steffen Klassert wrote:
>
> > Some quick benchmark numbers with UDP packet forwarding
> > (1460 byte packets) through two gateways:
> >
> > net-next: 16.4 Gbps
> >
> > net-next + UDP GRO: 20.3 Gbps
>
> uhmmm... what do you think about this speed-up ?
skb_segment() burns a lot of cycles. If I do the same test with
TCP and disable HW TSO, throughput drops also down to similar
values.
In case of software segmentation, the skb chain appropach
is likely faster because packets are not mangled. So no need
to allocate skbs, no new checksum calculations, less memcpy etc.
If we have an early route lookup in GRO, we could have a good
guess on the offload capabilities of the outgoing device.
So in case that software segmentation is likely, use the
skb chaining method. If HW segmentation is likely, merge
IP packets.
The chaining method might be also faster on non UDP GRO enabled
sockets.
I'll try to implement the skb chaining method on top of this
to see what we get from that.
^ permalink raw reply
* Re: [PATCH][next] igc: fix error return handling from call to netif_set_real_num_tx_queues
From: Neftin, Sasha @ 2018-10-24 10:38 UTC (permalink / raw)
To: Colin King, Jeff Kirsher, David S . Miller, intel-wired-lan
Cc: kernel-janitors, netdev, Neftin, Sasha
In-Reply-To: <20181019181615.27528-1-colin.king@canonical.com>
On 10/19/2018 21:16, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The call to netif_set_real_num_tx_queues is not assigning the error
> return to variable err even though the next line checks err for an
> error. Fix this by adding the missing err assignment.
>
> Detected by CoverityScan, CID#1474551 ("Logically dead code")
>
> Fixes: 3df25e4c1e66 ("igc: Add interrupt support")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
> drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 9d85707e8a81..80ddbd987764 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -3358,7 +3358,7 @@ static int __igc_open(struct net_device *netdev, bool resuming)
> goto err_req_irq;
>
> /* Notify the stack of the actual queue counts. */
> - netif_set_real_num_tx_queues(netdev, adapter->num_tx_queues);
> + err = netif_set_real_num_tx_queues(netdev, adapter->num_tx_queues);
> if (err)
> goto err_set_queues;
>
>
Thanks for the patch. Good catch of my typo.
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
^ permalink raw reply
* Re: [PATCH net-next] igc: Remove set but not used variables 'ctrl_ext, link_mode'
From: Neftin, Sasha @ 2018-10-24 10:20 UTC (permalink / raw)
To: YueHaibing, Jeff Kirsher
Cc: intel-wired-lan, netdev, kernel-janitors, Neftin, Sasha
In-Reply-To: <1539952830-187358-1-git-send-email-yuehaibing@huawei.com>
On 10/19/2018 15:40, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/ethernet/intel/igc/igc_base.c: In function 'igc_init_phy_params_base':
> drivers/net/ethernet/intel/igc/igc_base.c:240:6: warning:
> variable 'ctrl_ext' set but not used [-Wunused-but-set-variable]
> u32 ctrl_ext;
>
> drivers/net/ethernet/intel/igc/igc_base.c: In function 'igc_get_invariants_base':
> drivers/net/ethernet/intel/igc/igc_base.c:290:6: warning:
> variable 'link_mode' set but not used [-Wunused-but-set-variable]
> u32 link_mode = 0;
>
> It never used since introduction in
> commit c0071c7aa5fe ("igc: Add HW initialization code")
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> I'm not sure that reading IGC_CTRL_EXT is necessary.
> ---
> drivers/net/ethernet/intel/igc/igc_base.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_base.c b/drivers/net/ethernet/intel/igc/igc_base.c
> index 832da609..df40af7 100644
> --- a/drivers/net/ethernet/intel/igc/igc_base.c
> +++ b/drivers/net/ethernet/intel/igc/igc_base.c
> @@ -237,7 +237,6 @@ static s32 igc_init_phy_params_base(struct igc_hw *hw)
> {
> struct igc_phy_info *phy = &hw->phy;
> s32 ret_val = 0;
> - u32 ctrl_ext;
>
> if (hw->phy.media_type != igc_media_type_copper) {
> phy->type = igc_phy_none;
> @@ -247,8 +246,6 @@ static s32 igc_init_phy_params_base(struct igc_hw *hw)
> phy->autoneg_mask = AUTONEG_ADVERTISE_SPEED_DEFAULT_2500;
> phy->reset_delay_us = 100;
>
> - ctrl_ext = rd32(IGC_CTRL_EXT);
> -
> /* set lan id */
> hw->bus.func = (rd32(IGC_STATUS) & IGC_STATUS_FUNC_MASK) >>
> IGC_STATUS_FUNC_SHIFT;
> @@ -287,8 +284,6 @@ static s32 igc_init_phy_params_base(struct igc_hw *hw)
> static s32 igc_get_invariants_base(struct igc_hw *hw)
> {
> struct igc_mac_info *mac = &hw->mac;
> - u32 link_mode = 0;
> - u32 ctrl_ext = 0;
> s32 ret_val = 0;
>
> switch (hw->device_id) {
> @@ -302,9 +297,6 @@ static s32 igc_get_invariants_base(struct igc_hw *hw)
>
> hw->phy.media_type = igc_media_type_copper;
>
> - ctrl_ext = rd32(IGC_CTRL_EXT);
> - link_mode = ctrl_ext & IGC_CTRL_EXT_LINK_MODE_MASK;
> -
> /* mac initialization and operations */
> ret_val = igc_init_mac_params_base(hw);
> if (ret_val)
>
Thanks for the patch. Good.
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-24 18:28 UTC (permalink / raw)
To: David Miller, wanghaifine; +Cc: netdev, LKML
In-Reply-To: <20181024.101028.1211941922121897721.davem@davemloft.net>
On Wed, 2018-10-24 at 10:10 -0700, David Miller wrote:
> From: Wang Hai <wanghaifine@gmail.com>
> Date: Wed, 24 Oct 2018 23:47:29 +0800
>
> > To determine whether len is less than zero, it should be put before
> > the function min_t, because the return value of min_t is not likely
> > to be less than zero.
[]
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
[]
> > @@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> > struct net *net = sock_net(sk);
> > int val, len;
> >
> > + len = min_t(unsigned int, len, sizeof(int));
> > +
> > if (get_user(len, optlen))
> > return -EFAULT;
>
> You can't be serious?
I'm not personally taken aback by this but
there is the new Code of
Conduct to consider.
John McEnroe earned quite a bit of his
reputation as an 'enfant terrible' via a
similar statement.
https://www.youtube.com/watch?v=t0hK1wyrrAU
Perhaps a different word choice next time in
reply to submitters of ill-considered and/or
defective patches could be useful.
^ permalink raw reply
* Attribute failed policy validation
From: Marco Berizzi @ 2018-10-24 9:56 UTC (permalink / raw)
To: netdev
Hi Folks,
I'm getting this message after the upgrade from linux 4.18
to 4.19
root@Calimero:~# tc qdisc add dev eth0 root handle 1:0 hfsc default 1
Error: Attribute failed policy validation.
root@Calimero:~# lsmod | grep hfsc
sch_hfsc 24576 0
root@Calimero:~# tc -V
tc utility, iproute2-ss181023
Am I doing anything wrong?
^ permalink raw reply
* Re: [PATCH v2] bpf: btf: Fix a missing-check bug
From: Martin Lau @ 2018-10-24 18:22 UTC (permalink / raw)
To: Wenwen Wang
Cc: Kangjie Lu, Alexei Starovoitov, Daniel Borkmann,
open list:BPF (Safe dynamic programs and tools),
open list:BPF (Safe dynamic programs and tools)
In-Reply-To: <20181024172514.l33dsaqdvs5yewvm@kafai-mbp>
On Wed, Oct 24, 2018 at 05:26:23PM +0000, Martin Lau wrote:
> On Wed, Oct 24, 2018 at 08:00:19AM -0500, Wenwen Wang wrote:
> > In btf_parse(), the header of the user-space btf data 'btf_data' is firstly
> > parsed and verified through btf_parse_hdr(). In btf_parse_hdr(), the header
> > is copied from user-space 'btf_data' to kernel-space 'btf->hdr' and then
> > verified. If no error happens during the verification process, the whole
> > data of 'btf_data', including the header, is then copied to 'data' in
> > btf_parse(). It is obvious that the header is copied twice here. More
> > importantly, no check is enforced after the second copy to make sure the
> > headers obtained in these two copies are same. Given that 'btf_data'
> > resides in the user space, a malicious user can race to modify the header
> > between these two copies. By doing so, the user can inject inconsistent
> > data, which can cause undefined behavior of the kernel and introduce
> > potential security risk.
btw, I am working on a patch that copies the btf_data before parsing/verifying
the header. That should avoid this from happening but that will
require a bit more code churns for the bpf branch.
> >
> > To avoid the above issue, this patch copies the parsed header from
> > 'btf->hdr' to 'data'. The remaining part in 'data' is still copied from the
> > user-space 'btf_data'.
> LGTM.
>
> Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> >
> > Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> > ---
> > kernel/bpf/btf.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 378cef7..b52a834a 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -2152,6 +2152,7 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> > struct btf_verifier_env *env = NULL;
> > struct bpf_verifier_log *log;
> > struct btf *btf = NULL;
> > + u32 hdr_len;
> > u8 *data;
> > int err;
> >
> > @@ -2200,7 +2201,15 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> > btf->data_size = btf_data_size;
> > btf->nohdr_data = btf->data + btf->hdr.hdr_len;
> >
> > - if (copy_from_user(data, btf_data, btf_data_size)) {
> > + /*
> > + * The header at btf_data could be modified by a malicious user
> > + * after it is parsed. So we copy the parsed header here. The
> > + * remaining part is still copied from btf_data.
> > + */
> > + hdr_len = min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr));
> > + memcpy(data, &btf->hdr, hdr_len);
> > + if (copy_from_user(data + hdr_len, (u8 __user *)btf_data + hdr_len,
> > + btf_data_size - hdr_len)) {
> > err = -EFAULT;
> > goto errout;
> > }
> > --
> > 2.7.4
> >
^ permalink raw reply
* Re: [PATCH net-next] igc: Remove set but not used variable 'pci_using_dac'
From: Neftin, Sasha @ 2018-10-24 9:55 UTC (permalink / raw)
To: YueHaibing, Jeff Kirsher, David S. Miller
Cc: intel-wired-lan, netdev, kernel-janitors, Neftin, Sasha
In-Reply-To: <1539953291-189201-1-git-send-email-yuehaibing@huawei.com>
On 10/19/2018 15:48, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/ethernet/intel/igc/igc_main.c: In function 'igc_probe':
> drivers/net/ethernet/intel/igc/igc_main.c:3535:11: warning:
> variable 'pci_using_dac' set but not used [-Wunused-but-set-variable]
>
> It never used since introduction in commit
> d89f88419f99 ("igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support")
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/net/ethernet/intel/igc/igc_main.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index 9d85707..06a4afbe 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -3532,19 +3532,16 @@ static int igc_probe(struct pci_dev *pdev,
> struct net_device *netdev;
> struct igc_hw *hw;
> const struct igc_info *ei = igc_info_tbl[ent->driver_data];
> - int err, pci_using_dac;
> + int err;
>
> err = pci_enable_device_mem(pdev);
> if (err)
> return err;
>
> - pci_using_dac = 0;
> err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
> if (!err) {
> err = dma_set_coherent_mask(&pdev->dev,
> DMA_BIT_MASK(64));
> - if (!err)
> - pci_using_dac = 1;
> } else {
> err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
> if (err) {
>
Thanks for the patch. Good.
Acked-by: Sasha Neftin <sasha.neftin@intel.com>
^ permalink raw reply
* [net-next][PATCH] net/ipv4: fix a net leak
From: Li RongQing @ 2018-10-24 9:36 UTC (permalink / raw)
To: netdev, dsahern
put net when input a invalid ifindex, otherwise it will be leaked
Fixes: 5fcd266a9f64("net/ipv4: Add support for dumping addresses for a specific device")
Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Zhang Yu <zhangyu31@baidu.com>
Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
net/ipv4/devinet.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
index 63d5b58fbfdb..fd0c5a47e742 100644
--- a/net/ipv4/devinet.c
+++ b/net/ipv4/devinet.c
@@ -1775,8 +1775,10 @@ static int inet_dump_ifaddr(struct sk_buff *skb, struct netlink_callback *cb)
if (fillargs.ifindex) {
dev = __dev_get_by_index(tgt_net, fillargs.ifindex);
- if (!dev)
+ if (!dev) {
+ put_net(tgt_net);
return -ENODEV;
+ }
in_dev = __in_dev_get_rtnl(dev);
if (in_dev) {
--
2.16.2
^ permalink raw reply related
* Re: [PATCH] bpf: btf: Fix a missing-check bug
From: Daniel Borkmann @ 2018-10-24 9:16 UTC (permalink / raw)
To: Y Song, wang6495; +Cc: kjlu, Alexei Starovoitov, netdev, LKML
In-Reply-To: <CAH3MdRV2qLj_3KHFYmCKYHMx2wZk_4mQrdvRJe+znXr3Astm8g@mail.gmail.com>
Hi Wenwen,
On 10/22/2018 05:57 PM, Y Song wrote:
> On Fri, Oct 19, 2018 at 3:30 PM Wenwen Wang <wang6495@umn.edu> wrote:
>>
>> In btf_parse(), the header of the user-space btf data 'btf_data' is firstly
>> parsed and verified through btf_parse_hdr(). In btf_parse_hdr(), the header
>> is copied from user-space 'btf_data' to kernel-space 'btf->hdr' and then
>> verified. If no error happens during the verification process, the whole
>> data of 'btf_data', including the header, is then copied to 'data' in
>> btf_parse(). It is obvious that the header is copied twice here. More
>> importantly, no check is enforced after the second copy to make sure the
>> headers obtained in these two copies are same. Given that 'btf_data'
>> resides in the user space, a malicious user can race to modify the header
>> between these two copies. By doing so, the user can inject inconsistent
>> data, which can cause undefined behavior of the kernel and introduce
>> potential security risk.
>>
>> To avoid the above issue, this patch rewrites the header after the second
>> copy, using 'btf->hdr', which is obtained in the first copy.
>>
>> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
>> ---
>> kernel/bpf/btf.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
>> index 138f030..2a85f91 100644
>> --- a/kernel/bpf/btf.c
>> +++ b/kernel/bpf/btf.c
>> @@ -2202,6 +2202,9 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
>> goto errout;
>> }
>>
>> + memcpy(data, &btf->hdr,
>> + min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr)));
>
> Could you restructure the code to memcpy the header followed by copying
> the rest of btf_data with copy_from_user? This way, each byte is only
> copied once.
> Could you add some comments right before memcpy so later people will know
> why we implement this way?
Thanks for the fix! Agree with Yonghong that we should rework this a bit, so
please respin a v2 with the feedback addressed, thanks.
>> +
>> err = btf_parse_str_sec(env);
>> if (err)
>> goto errout;
>> --
>> 2.7.4
>>
^ permalink raw reply
* Re: [PATCH bpf] bpf: devmap: fix wrong interface selection in notifier_call
From: Daniel Borkmann @ 2018-10-24 9:07 UTC (permalink / raw)
To: Taehee Yoo, ast; +Cc: netdev, john.fastabend
In-Reply-To: <20181022154143.15396-1-ap420073@gmail.com>
[ +John ]
On 10/22/2018 05:41 PM, Taehee Yoo wrote:
> The dev_map_notification() removes interface in devmap if
> unregistering interface's ifindex is same.
> But only checking ifindex is not enough because other netns can have
> same ifindex. so that wrong interface selection could occurred.
> Hence the net_eq() is needed.
>
> Fixes: 2ddf71e23cc2 ("net: add notifier hooks for devmap bpf map")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> kernel/bpf/devmap.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index 141710b82a6c..0d9211e49a4a 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -496,6 +496,7 @@ static int dev_map_notification(struct notifier_block *notifier,
> ulong event, void *ptr)
> {
> struct net_device *netdev = netdev_notifier_info_to_dev(ptr);
> + struct net *net = dev_net(netdev);
> struct bpf_dtab *dtab;
> int i;
>
> @@ -512,7 +513,7 @@ static int dev_map_notification(struct notifier_block *notifier,
> struct bpf_dtab_netdev *dev, *odev;
>
> dev = READ_ONCE(dtab->netdev_map[i]);
> - if (!dev ||
> + if (!dev || !net_eq(net, dev_net(dev->dev)) ||
> dev->dev->ifindex != netdev->ifindex)
Can't we just compare netdev pointers directly here?
> continue;
> odev = cmpxchg(&dtab->netdev_map[i], dev, NULL);
>
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: kbuild test robot @ 2018-10-24 17:34 UTC (permalink / raw)
To: Wang Hai
Cc: kbuild-all, edumazet, davem, kuznet, yoshfuji, netdev,
linux-kernel, Wang Hai
In-Reply-To: <20181024154729.5312-1-wanghaifine@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]
Hi Wang,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v4.19]
[also build test WARNING on next-20181019]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Wang-Hai/Change-judgment-len-position/20181025-010812
config: i386-randconfig-x002-201842 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/compiler_types.h:64:0,
from <command-line>:0:
net/ipv4/tcp.c: In function 'do_tcp_getsockopt':
>> include/linux/compiler-gcc.h:82:45: warning: 'len' is used uninitialized in this function [-Wuninitialized]
#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
^~~~~~~~~~~~
net/ipv4/tcp.c:3302:11: note: 'len' was declared here
int val, len;
^~~
--
In file included from include/linux/compiler_types.h:64:0,
from <command-line>:0:
net//ipv4/tcp.c: In function 'do_tcp_getsockopt':
>> include/linux/compiler-gcc.h:82:45: warning: 'len' is used uninitialized in this function [-Wuninitialized]
#define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
^~~~~~~~~~~~
net//ipv4/tcp.c:3302:11: note: 'len' was declared here
int val, len;
^~~
vim +/len +82 include/linux/compiler-gcc.h
87358710 David Woodhouse 2018-02-19 81
cb984d10 Joe Perches 2015-06-25 @82 #define __UNIQUE_ID(prefix) __PASTE(__PASTE(__UNIQUE_ID_, prefix), __COUNTER__)
cb984d10 Joe Perches 2015-06-25 83
:::::: The code at line 82 was first introduced by commit
:::::: cb984d101b30eb7478d32df56a0023e4603cba7f compiler-gcc: integrate the various compiler-gcc[345].h files
:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26475 bytes --]
^ permalink raw reply
* [RFC] [PATCH] netfilter: Fix kmemleak false positive reports
From: zhe.he @ 2018-10-24 17:29 UTC (permalink / raw)
To: pablo, kadlec, fw, davem, netfilter-devel, coreteam, netdev,
linux-kernel, catalin.marinas, linux-mm, zhe.he
From: He Zhe <zhe.he@windriver.com>
unreferenced object 0xffff9643edb89900 (size 256):
comm "sd-resolve", pid 220, jiffies 4295016710 (age 208.256s)
hex dump (first 32 bytes):
01 00 00 00 00 00 00 00 03 00 74 f3 ba b1 b6 b5 ..........t.....
65 3e 00 00 00 00 00 00 90 f9 a0 ed 43 96 ff ff e>..........C...
backtrace:
[<0000000070d5b185>] kmem_cache_alloc+0x146/0x200
[<0000000007a27faa>] __nf_conntrack_alloc.isra.13+0x4d/0x170 [nf_conntrack]
[<00000000ecc5b0ec>] init_conntrack+0x6a/0x2f0 [nf_conntrack]
[<000000003d38809f>] nf_conntrack_in+0x2c5/0x360 [nf_conntrack]
[<000000001fe154e3>] ipv4_conntrack_local+0x5d/0x70 [nf_conntrack_ipv4]
[<0000000027adadb2>] nf_hook_slow+0x48/0xd0
[<000000009893511f>] __ip_local_out+0xbd/0xf0
[<00000000d68cbd2f>] ip_local_out+0x1c/0x50
[<00000000995e2f37>] ip_send_skb+0x19/0x40
[<000000003d95f220>] udp_send_skb.isra.5+0x157/0x360
[<00000000ebc25968>] udp_sendmsg+0x9d8/0xc10
[<000000003bef56ec>] inet_sendmsg+0x3e/0xf0
[<000000008d23e405>] sock_sendmsg+0x1d/0x30
[<000000008c297097>] ___sys_sendmsg+0x108/0x2b0
[<00000000f15a806c>] __sys_sendmmsg+0xba/0x1c0
[<00000000e195d2cf>] __x64_sys_sendmmsg+0x24/0x30
In __nf_conntrack_confirm, object ct can be referenced to by the stack variable
ct and the members of ct->tuplehash. kmemleak needs at least one of them to find
the ct object during scan.
When the ct object is moved from the unconfirmed hlist to the confirmed hlist.
kmemleak cannot see ct object if things happen in the following order and thus
give the above false positive report.
1) The ct object is removed from the unconfirmed hlist.
2) kmemleak scans data/bss sections(heap scan passes without heap reference).
3) The ct object is added to confirmed hlist and the variable ct is destroyed as
the function returns.
4) kmemleak scans task stacks(stack scan passes without stack reference).
This patch marks ct object as not a leak.
Signed-off-by: He Zhe <zhe.he@windriver.com>
Cc: pablo@netfilter.org
Cc: kadlec@blackhole.kfki.hu
Cc: fw@strlen.de
Cc: davem@davemloft.net
Cc: catalin.marinas@arm.com
---
So far this is only observed in v4.18, not in v4.19. But the case seems apply
to both.
net/netfilter/nf_conntrack_core.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
index a676d5f..067365d 100644
--- a/net/netfilter/nf_conntrack_core.c
+++ b/net/netfilter/nf_conntrack_core.c
@@ -35,6 +35,7 @@
#include <linux/mm.h>
#include <linux/nsproxy.h>
#include <linux/rculist_nulls.h>
+#include <linux/kmemleak.h>
#include <net/netfilter/nf_conntrack.h>
#include <net/netfilter/nf_conntrack_l4proto.h>
@@ -1282,6 +1283,8 @@ __nf_conntrack_alloc(struct net *net,
if (ct == NULL)
goto out;
+ kmemleak_not_leak(ct);
+
spin_lock_init(&ct->lock);
ct->tuplehash[IP_CT_DIR_ORIGINAL].tuple = *orig;
ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode.pprev = NULL;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] bpf: btf: Fix a missing-check bug
From: Martin Lau @ 2018-10-24 17:26 UTC (permalink / raw)
To: Wenwen Wang
Cc: Kangjie Lu, Alexei Starovoitov, Daniel Borkmann,
open list:BPF (Safe dynamic programs and tools),
open list:BPF (Safe dynamic programs and tools)
In-Reply-To: <1540386020-30680-1-git-send-email-wang6495@umn.edu>
On Wed, Oct 24, 2018 at 08:00:19AM -0500, Wenwen Wang wrote:
> In btf_parse(), the header of the user-space btf data 'btf_data' is firstly
> parsed and verified through btf_parse_hdr(). In btf_parse_hdr(), the header
> is copied from user-space 'btf_data' to kernel-space 'btf->hdr' and then
> verified. If no error happens during the verification process, the whole
> data of 'btf_data', including the header, is then copied to 'data' in
> btf_parse(). It is obvious that the header is copied twice here. More
> importantly, no check is enforced after the second copy to make sure the
> headers obtained in these two copies are same. Given that 'btf_data'
> resides in the user space, a malicious user can race to modify the header
> between these two copies. By doing so, the user can inject inconsistent
> data, which can cause undefined behavior of the kernel and introduce
> potential security risk.
>
> To avoid the above issue, this patch copies the parsed header from
> 'btf->hdr' to 'data'. The remaining part in 'data' is still copied from the
> user-space 'btf_data'.
LGTM.
Acked-by: Martin KaFai Lau <kafai@fb.com>
>
> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
> kernel/bpf/btf.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> index 378cef7..b52a834a 100644
> --- a/kernel/bpf/btf.c
> +++ b/kernel/bpf/btf.c
> @@ -2152,6 +2152,7 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> struct btf_verifier_env *env = NULL;
> struct bpf_verifier_log *log;
> struct btf *btf = NULL;
> + u32 hdr_len;
> u8 *data;
> int err;
>
> @@ -2200,7 +2201,15 @@ static struct btf *btf_parse(void __user *btf_data, u32 btf_data_size,
> btf->data_size = btf_data_size;
> btf->nohdr_data = btf->data + btf->hdr.hdr_len;
>
> - if (copy_from_user(data, btf_data, btf_data_size)) {
> + /*
> + * The header at btf_data could be modified by a malicious user
> + * after it is parsed. So we copy the parsed header here. The
> + * remaining part is still copied from btf_data.
> + */
> + hdr_len = min_t(u32, btf->hdr.hdr_len, sizeof(btf->hdr));
> + memcpy(data, &btf->hdr, hdr_len);
> + if (copy_from_user(data + hdr_len, (u8 __user *)btf_data + hdr_len,
> + btf_data_size - hdr_len)) {
> err = -EFAULT;
> goto errout;
> }
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-24 17:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: Willy Tarreau, wanghaifine, David Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, netdev, LKML
In-Reply-To: <CANn89iLH=VP1i=KS5QV1x41EpQM5-o1TJfDh01Y++bMpFpfBRg@mail.gmail.com>
On Wed, 2018-10-24 at 10:03 -0700, Eric Dumazet wrote:
> On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <joe@perches.com> wrote:
>
> > I think if the point is to test for negative numbers,
> > it's clearer to do that before using min_t.and it's
> > probably clearer not to use min_t at all.
> >
>
> ...
>
> > if (len > sizeof(int))
> > len = sizeof(int);
>
> It is a matter of taste really,
Agree and hence my use of 'I think' above.
> I know some people (like me) sometimes
> mixes min() and max()
Not quite sure what you mean here by mixes.
mix up? If so, the < > inversions probably
have about the same error rate.
And I suppose there are cases where the
always set of len in uses like
len = min(len, 4);
are more costly (len being in a slow write
speed area of memory or some such) than the
other style of
if (len < 4)
len = 4;
I think that min() is easier to read in most
cases.
> I would suggest that if someones wants to change the current code, a
> corresponding test would be added in tools/testing/selftests/net?
^ permalink raw reply
* Re: [net-next 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support
From: Neftin, Sasha @ 2018-10-24 8:50 UTC (permalink / raw)
To: Jakub Kicinski, Jeff Kirsher
Cc: davem, netdev, nhorman, sassmann, sasha.neftin
In-Reply-To: <20181018101405.4998dc01@cakuba.netronome.com>
On 10/18/2018 20:14, Jakub Kicinski wrote:
> On Wed, 17 Oct 2018 15:23:12 -0700, Jeff Kirsher wrote:
>> From: Sasha Neftin <sasha.neftin@intel.com>
>>
>> This patch adds the beginning framework onto which I am going to add
>> the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
>> Ethernet Controller.
>>
>> Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
>> Tested-by: Aaron Brown <aaron.f.brown@intel.com>
>> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
>
> bunch of minor nit picks
>
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
>> new file mode 100644
>> index 000000000000..afe595cfcf63
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2018 Intel Corporation */
>> +
>> +#ifndef _IGC_H_
>> +#define _IGC_H_
>> +
>> +#include <linux/kobject.h>
>> +
>> +#include <linux/pci.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/vmalloc.h>
>> +
>> +#include <linux/ethtool.h>
>> +
>> +#include <linux/sctp.h>
>> +
>> +#define IGC_ERR(args...) pr_err("igc: " args)
>
> Looks like you're reimplementing pr_fmt()
>
yes, it is convenient for a debug. Same methodological approach I saw in
other drivers.
>> +#define PFX "igc: "
>> +
>> +#include <linux/timecounter.h>
>> +#include <linux/net_tstamp.h>
>> +#include <linux/ptp_clock_kernel.h>
>
> Splitting the includes looks fairly weird. Also, you're not using any
> of them here.
>
Good catch. I'll remove splits and unused "include"'s. All "include"'s
will be add per demand. I will send patch to fix that.
>> +/* main */
>> +extern char igc_driver_name[];
>> +extern char igc_driver_version[];
>> +
>> +#endif /* _IGC_H_ */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h
>> new file mode 100644
>> index 000000000000..aa68b4516700
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/igc/igc_hw.h
>> @@ -0,0 +1,10 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (c) 2018 Intel Corporation */
>> +
>> +#ifndef _IGC_HW_H_
>> +#define _IGC_HW_H_
>> +
>> +#define IGC_DEV_ID_I225_LM 0x15F2
>> +#define IGC_DEV_ID_I225_V 0x15F3
>> +
>> +#endif /* _IGC_HW_H_ */
>> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
>> new file mode 100644
>> index 000000000000..753749ce5ae0
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
>> @@ -0,0 +1,146 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* Copyright (c) 2018 Intel Corporation */
>> +
>> +#include <linux/module.h>
>> +#include <linux/types.h>
>> +
>> +#include "igc.h"
>> +#include "igc_hw.h"
>> +
>> +#define DRV_VERSION "0.0.1-k"
>
> You can just use the kernel version, it works pretty well in presence
> of backports.
>
Good idea, I think I will do it too.
>> +#define DRV_SUMMARY "Intel(R) 2.5G Ethernet Linux Driver"
>> +
>> +MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
>> +MODULE_DESCRIPTION(DRV_SUMMARY);
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_VERSION(DRV_VERSION);
>> +
>> +char igc_driver_name[] = "igc";
>> +char igc_driver_version[] = DRV_VERSION;
>> +static const char igc_driver_string[] = DRV_SUMMARY;
>> +static const char igc_copyright[] =
>> + "Copyright(c) 2018 Intel Corporation.";
>> +
>> +static const struct pci_device_id igc_pci_tbl[] = {
>> + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) },
>> + { PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) },
>> + /* required last entry */
>> + {0, }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(pci, igc_pci_tbl);
>> +
>> +/**
>> + * igc_probe - Device Initialization Routine
>> + * @pdev: PCI device information struct
>> + * @ent: entry in igc_pci_tbl
>> + *
>> + * Returns 0 on success, negative on failure
>> + *
>> + * igc_probe initializes an adapter identified by a pci_dev structure.
>> + * The OS initialization, configuring the adapter private structure,
>> + * and a hardware reset occur.
>> + */
>> +static int igc_probe(struct pci_dev *pdev,
>> + const struct pci_device_id *ent)
>> +{
>> + int err, pci_using_dac;
>> +
>> + err = pci_enable_device_mem(pdev);
>> + if (err)
>> + return err;
>> +
>> + pci_using_dac = 0;
>> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
>> + if (!err) {
>> + err = dma_set_coherent_mask(&pdev->dev,
>> + DMA_BIT_MASK(64));
>> + if (!err)
>> + pci_using_dac = 1;
>
> You never use this pci_using_dac. dma_set_mask_and_coherent() maybe?
>
Right. I saw already somebody sent out patch to fix that.
>> + } else {
>> + err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
>> + if (err) {
>> + err = dma_set_coherent_mask(&pdev->dev,
>> + DMA_BIT_MASK(32));
>> + if (err) {
>> + IGC_ERR("Wrong DMA configuration, aborting\n");
>> + goto err_dma;
>> + }
>> + }
>> + }
>> +
>> + err = pci_request_selected_regions(pdev,
>> + pci_select_bars(pdev,
>> + IORESOURCE_MEM),
>> + igc_driver_name);
>> + if (err)
>> + goto err_pci_reg;
>> +
>> + pci_set_master(pdev);
>> + err = pci_save_state(pdev);
>
> And you never look at err?
>
Same as above.
>> + return 0;
>> +
>> +err_pci_reg:
>> +err_dma:
>
> The error label should be named after what it points to, not where it's
> coming from.
>
>> + pci_disable_device(pdev);
>> + return err;
>> +}
>> +
>> +/**
>> + * igc_remove - Device Removal Routine
>> + * @pdev: PCI device information struct
>> + *
>> + * igc_remove is called by the PCI subsystem to alert the driver
>> + * that it should release a PCI device. This could be caused by a
>> + * Hot-Plug event, or because the driver is going to be removed from
>> + * memory.
>> + */
>> +static void igc_remove(struct pci_dev *pdev)
>> +{
>> + pci_release_selected_regions(pdev,
>> + pci_select_bars(pdev, IORESOURCE_MEM));
>> +
>> + pci_disable_device(pdev);
>> +}
>> +
>> +static struct pci_driver igc_driver = {
>> + .name = igc_driver_name,
>> + .id_table = igc_pci_tbl,
>> + .probe = igc_probe,
>> + .remove = igc_remove,
>> +};
>> +
>> +/**
>> + * igc_init_module - Driver Registration Routine
>> + *
>> + * igc_init_module is the first routine called when the driver is
>> + * loaded. All it does is register with the PCI subsystem.
>> + */
>> +static int __init igc_init_module(void)
>> +{
>> + int ret;
>> +
>> + pr_info("%s - version %s\n",
>> + igc_driver_string, igc_driver_version);
>> +
>> + pr_info("%s\n", igc_copyright);
>> +
>> + ret = pci_register_driver(&igc_driver);
>> + return ret;
>
> Why the variable?
>
we can return 'pci_register_driver' here, but variable is more clearly.
>> +}
>> +
>> +module_init(igc_init_module);
>> +
>> +/**
>> + * igc_exit_module - Driver Exit Cleanup Routine
>> + *
>> + * igc_exit_module is called just before the driver is removed
>> + * from memory.
>> + */
>> +static void __exit igc_exit_module(void)
>> +{
>> + pci_unregister_driver(&igc_driver);
>> +}
>> +
>> +module_exit(igc_exit_module);
>> +/* igc_main.c */
>
> I'd argue most editors make it fairly clear which file one is
> editing, hence making this sort of comments entirely superfluous :)
>
Thanks for your comments.
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: David Miller @ 2018-10-24 17:10 UTC (permalink / raw)
To: wanghaifine; +Cc: edumazet, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20181024154729.5312-1-wanghaifine@gmail.com>
From: Wang Hai <wanghaifine@gmail.com>
Date: Wed, 24 Oct 2018 23:47:29 +0800
> To determine whether len is less than zero, it should be put before
> the function min_t, because the return value of min_t is not likely
> to be less than zero.
>
> Signed-off-by: Wang Hai <wanghaifine@gmail.com>
> ---
> net/ipv4/tcp.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 10c624639..49af9fdc3 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3301,11 +3301,11 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> struct net *net = sock_net(sk);
> int val, len;
>
> + len = min_t(unsigned int, len, sizeof(int));
> +
> if (get_user(len, optlen))
> return -EFAULT;
You can't be serious?
'len' has no value before the get_user() call.
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Eric Dumazet @ 2018-10-24 17:03 UTC (permalink / raw)
To: joe
Cc: Willy Tarreau, wanghaifine, David Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, netdev, LKML
In-Reply-To: <bbc4eba31b89cb487962c0a5980a12c53b1aa58b.camel@perches.com>
On Wed, Oct 24, 2018 at 9:54 AM Joe Perches <joe@perches.com> wrote:
> I think if the point is to test for negative numbers,
> it's clearer to do that before using min_t.and it's
> probably clearer not to use min_t at all.
>
...
>
> if (len > sizeof(int))
> len = sizeof(int);
It is a matter of taste really, I know some people (like me) sometimes
mixes min() and max()
I would suggest that if someones wants to change the current code, a
corresponding test
would be added in tools/testing/selftests/net
^ permalink raw reply
* [PATCH net-next] octeontx2-af: Copy the right amount of memory
From: Dan Carpenter @ 2018-10-24 8:32 UTC (permalink / raw)
To: Sunil Goutham
Cc: Linu Cherian, Geetha sowjanya, Jerin Jacob, David S. Miller,
netdev, kernel-janitors
This is a copy and paste bug where we copied the sizeof() from the chunk
before. We're copying more data than intended but the destination is a
union so it doesn't cause memory corruption.
Fixes: ffb0abd7e9cb ("octeontx2-af: NIX AQ instruction enqueue support")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
index 8890c95831ca..a4eac3b9ee72 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
+++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu_nix.c
@@ -573,7 +573,7 @@ static int rvu_nix_aq_enq_inst(struct rvu *rvu, struct nix_aq_enq_req *req,
sizeof(struct nix_cq_ctx_s));
else if (req->ctype == NIX_AQ_CTYPE_RSS)
memcpy(&rsp->rss, ctx,
- sizeof(struct nix_cq_ctx_s));
+ sizeof(struct nix_rsse_s));
else if (req->ctype == NIX_AQ_CTYPE_MCE)
memcpy(&rsp->mce, ctx,
sizeof(struct nix_rx_mce_s));
--
2.11.0
^ permalink raw reply related
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-24 16:54 UTC (permalink / raw)
To: Willy Tarreau
Cc: Wang Hai, edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20181024163230.GA25382@1wt.eu>
On Wed, 2018-10-24 at 18:32 +0200, Willy Tarreau wrote:
> On Wed, Oct 24, 2018 at 09:23:19AM -0700, Joe Perches wrote:
> > On Wed, 2018-10-24 at 17:57 +0200, Willy Tarreau wrote:
> > > On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> > > > To determine whether len is less than zero, it should be put before
> > > > the function min_t, because the return value of min_t is not likely
> > > > to be less than zero.
> > >
> > > Huh? First, the <0 test is made on "len", not "min_t", so it still
> > > is signed. Second, you're in fact completely removing the test here,
> > > look :
> > >
> > > > struct net *net = sock_net(sk);
> > > > int val, len;
> > > >
> > > > + len = min_t(unsigned int, len, sizeof(int));
> > > > +
> > >
> > > len is used uninitialized here, so the result is undefined.
> > >
> > > > if (get_user(len, optlen))
> > > > return -EFAULT;
> > >
> > > Then it gets overridden by get_user()
> > >
> > > > - len = min_t(unsigned int, len, sizeof(int));
> > > > -
> > >
> > > Then its positive values are not bounded anymore since you moved the test.
> >
> > Not quite.
> >
> > Problem here is negative values are tested as
> > large positive values and limited to 4
> >
> > ie:
> > ien len = -1,
> > len = min_t(unsigned int, len, sizeof(int));
> >
> > len is now 4
> >
> > > > if (len < 0)
> > > > return -EINVAL;
> >
> > So this test len < 0 could be moved up above min_t
>
> It could indeed, or we could also have min_t() done on an int instead
> of an unsigned int and this would avoid the need to shuffle the code
> around and open a huge hole like this one.
I think if the point is to test for negative numbers,
it's clearer to do that before using min_t.and it's
probably clearer not to use min_t at all.
if (get_user(len, optlen))
return -EFAULT;
if (len < 0)
return -EINVAL;
if (len > sizeof(int))
len = sizeof(int);
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Willy Tarreau @ 2018-10-24 16:32 UTC (permalink / raw)
To: Joe Perches
Cc: Wang Hai, edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <60f08664db5751949ddfb34666bfda77f99682f1.camel@perches.com>
On Wed, Oct 24, 2018 at 09:23:19AM -0700, Joe Perches wrote:
> On Wed, 2018-10-24 at 17:57 +0200, Willy Tarreau wrote:
> > On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> > > To determine whether len is less than zero, it should be put before
> > > the function min_t, because the return value of min_t is not likely
> > > to be less than zero.
> >
> > Huh? First, the <0 test is made on "len", not "min_t", so it still
> > is signed. Second, you're in fact completely removing the test here,
> > look :
> >
> > > struct net *net = sock_net(sk);
> > > int val, len;
> > >
> > > + len = min_t(unsigned int, len, sizeof(int));
> > > +
> >
> > len is used uninitialized here, so the result is undefined.
> >
> > > if (get_user(len, optlen))
> > > return -EFAULT;
> >
> > Then it gets overridden by get_user()
> >
> > > - len = min_t(unsigned int, len, sizeof(int));
> > > -
> >
> > Then its positive values are not bounded anymore since you moved the test.
>
> Not quite.
>
> Problem here is negative values are tested as
> large positive values and limited to 4
>
> ie:
> ien len = -1,
> len = min_t(unsigned int, len, sizeof(int));
>
> len is now 4
>
> > > if (len < 0)
> > > return -EINVAL;
>
> So this test len < 0 could be moved up above min_t
It could indeed, or we could also have min_t() done on an int instead
of an unsigned int and this would avoid the need to shuffle the code
around and open a huge hole like this one.
Willy
^ permalink raw reply
* Re: [PATCH] Change judgment len position
From: Joe Perches @ 2018-10-24 16:23 UTC (permalink / raw)
To: Willy Tarreau, Wang Hai
Cc: edumazet, davem, kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20181024155739.GA25314@1wt.eu>
On Wed, 2018-10-24 at 17:57 +0200, Willy Tarreau wrote:
> On Wed, Oct 24, 2018 at 11:47:29PM +0800, Wang Hai wrote:
> > To determine whether len is less than zero, it should be put before
> > the function min_t, because the return value of min_t is not likely
> > to be less than zero.
>
> Huh? First, the <0 test is made on "len", not "min_t", so it still
> is signed. Second, you're in fact completely removing the test here,
> look :
>
> > struct net *net = sock_net(sk);
> > int val, len;
> >
> > + len = min_t(unsigned int, len, sizeof(int));
> > +
>
> len is used uninitialized here, so the result is undefined.
>
> > if (get_user(len, optlen))
> > return -EFAULT;
>
> Then it gets overridden by get_user()
>
> > - len = min_t(unsigned int, len, sizeof(int));
> > -
>
> Then its positive values are not bounded anymore since you moved the test.
Not quite.
Problem here is negative values are tested as
large positive values and limited to 4
ie:
ien len = -1,
len = min_t(unsigned int, len, sizeof(int));
len is now 4
> > if (len < 0)
> > return -EINVAL;
So this test len < 0 could be moved up above min_t
> Then only negative values are dropped. So unless I'm missing something
> obvious, you're just allowing len to be as large as 2GB-1 based on the
> user's fed optlen.
>
> Am I wrong ?
>
> Willychee
^ permalink raw reply
* Re: [GIT] Networking
From: Kalle Valo @ 2018-10-24 7:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: Bjorn Andersson, Govind Singh, David Miller, Andrew Morton,
netdev, Linux Kernel Mailing List, linux-wireless, ath10k,
Jeff Kirsher, Niklas Cassel, Andy Gross, David Brown
In-Reply-To: <CAHk-=wh2H9jE2RXibQgWs3_Q539roj62_gzPXKjnhhKqSFcw2g@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Oct 24, 2018 at 7:01 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>>
>> Hmm. Tentatively pulled, but there's something wrong with the Kconfig rules.
>
> Confirmed.
BTW, our emails crossed and more info in the other email[1].
> I did a978a5b8d83f ("net/kconfig: Make QCOM_QMI_HELPERS available when
> COMPILE_TEST") to fix the breakage.
Thanks, though I don't see it yet as I guess you haven't pushed it yet.
Do note that it _might_ conflict the other commit which I suspect is in
also coming to you:
ccfb464cd106 ("soc: qcom: Allow COMPILE_TEST of qcom SoC Kconfigs")
> Why wasn't this noticed in testing?
Mostly bad timing due to my vacation. I did do allmodconfig build but
not sure why I missed the warning, also the kbuild bot didn't report
anything. Jeff did report[2] it last week but I was on vacation at the
time and just came back yesterday and didn't have time to react to it
yet. Really sorry for the mess.
[1] https://lkml.kernel.org/r/87pnvzu7i6.fsf@codeaurora.org
[2] http://lists.infradead.org/pipermail/ath10k/2018-October/012330.html
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH net-next 3/4] net: phy-c45: Implement reset/suspend/resume callbacks
From: Jose Abreu @ 2018-10-24 7:50 UTC (permalink / raw)
To: Russell King - ARM Linux, Jose Abreu
Cc: Florian Fainelli, Andrew Lunn, netdev, David S. Miller,
Joao Pinto
In-Reply-To: <20181023105828.GN30658@n2100.armlinux.org.uk>
On 23-10-2018 11:58, Russell King - ARM Linux wrote:
> On Tue, Oct 23, 2018 at 11:28:09AM +0100, Jose Abreu wrote:
>> On 23-10-2018 11:20, Russell King - ARM Linux wrote:
>>> I have no idea what you're proposing there - your patches weren't copied
>>> to me.
>> They just set / unset MDIO_CTRL1_LPOWER bit in PCS. I find that
>> without this remote end doesn't detect link is down ...
>>
>> If it's okay for Generic 10G driver I can submit only this and
>> manually reset PHY in stmmac driver so that I don't need to
>> implement custom PHY driver ...
>
>
>> BTW, I just found out currently Generic 10G Driver is broken
>> without patch 4/4 of this series [1]
>>
>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.ozlabs.org_patch_987570_&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=WHDsc6kcWAl4i96Vm5hJ_19IJiuxx_p_Rzo2g-uHDKw&m=zYEDSMZPTCHiPc_3B8buyu0kXnlIEYawnHWAxPrsoSU&s=MlF6I2cBSYkGxgEwNV-hXpXJIvXv_gRYXP-CazjkUSw&e=
> How is it broken - what are the symptoms?
>
> The generic 10G driver is bound not via the normal bus matching and
> phy_bus_match(), but via a manual bind in phy_attach_direct(). This
> calls the probe function, which is phy_probe(), which initialises
> the supported/advertising to the driver's features (which as you note
> are zero.)
Since 719655a14971 ("net: phy: Replace phy driver features u32
with link_mode bitmap"), phy_probe() calls
ethtool_convert_link_mode_to_legacy_u32() with phydrv->features
as argument. Since features are NULL, we will get NULL pointer
dereference.
I guess Generic 10G driver was forgotten in the conversion.
Thanks and Best Regards,
Jose Miguel Abreu
>
> However, phy_attach_direct() goes on to call phy_init_hw(), which
> calls the config_init() method. The config_init() method initialises
> the supported/advertising masks to 10GbaseT. This is (partly) what
> I refer to when I say that the generic 10G support is crippled - it
> only supports this single speed and media.
>
> So the supported/advertising masks should be forced to only 10GbaseT
> at the completion of phy_attach_direct().
>
> The "generic 10G" support doesn't do autonegotiation, configuration
> or link mode forcing. It only assumes 10GbaseT is supported, and
> only checks for the "link up" bits.
>
> It isn't like the non-10G generic PHY support due to history - it
> was added in 2014 by Andy Fleming (see 124059fd53af).
>
> BTW, your patch 1 is wrong as well (introducing phy_update_link()).
> You don't take account that a 10G phy may have alternative ways of
> reading the link (like 88x3310 does, because it has multiple
> instances of AN/PCS/PHYXS at 1k offsets.) All the gen10g_*
> functions are legacy functions for the crippled "generic" 10G
> support.
>
^ 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