* Re: [PATCH] net: stmmac: make "snps,reset-delays-us" optional again
From: David Miller @ 2019-07-02 22:23 UTC (permalink / raw)
To: martin.blumenstingl
Cc: netdev, colin.king, peppe.cavallaro, alexandre.torgue, joabreu,
linux-arm-kernel, linux-kernel
In-Reply-To: <20190701224225.19701-1-martin.blumenstingl@googlemail.com>
From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date: Tue, 2 Jul 2019 00:42:25 +0200
> Commit 760f1dc2958022 ("net: stmmac: add sanity check to
> device_property_read_u32_array call") introduced error checking of the
> device_property_read_u32_array() call in stmmac_mdio_reset().
> This results in the following error when the "snps,reset-delays-us"
> property is not defined in devicetree:
> invalid property snps,reset-delays-us
>
> This sanity check made sense until commit 84ce4d0f9f55b4 ("net: stmmac:
> initialize the reset delay array") ensured that there are fallback
> values for the reset delay if the "snps,reset-delays-us" property is
> absent. That was at the cost of making that property mandatory though.
>
> Drop the sanity check for device_property_read_u32_array() and thus make
> the "snps,reset-delays-us" property optional again (avoiding the error
> message while loading the stmmac driver with a .dtb where the property
> is absent).
>
> Fixes: 760f1dc2958022 ("net: stmmac: add sanity check to device_property_read_u32_array call")
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
> This is a fix for a patch in net-next and should either go into net-next
> or 5.3-rcX.
Ok, applied to net-next.
^ permalink raw reply
* Re: [PATCH V2] net: usb: asix: init MAC address buffers
From: David Miller @ 2019-07-02 22:25 UTC (permalink / raw)
To: tranmanphong
Cc: dcbw, netdev, linux-usb, glider, linux-kernel-mentees,
linux-kernel, lynxis, marcel.ziswiler, skhan,
syzbot+8a3fc6674bbc3978ed4e, syzkaller-bugs, yang.wei9, zhang.run
In-Reply-To: <20190702001008.26048-1-tranmanphong@gmail.com>
From: Phong Tran <tranmanphong@gmail.com>
Date: Tue, 2 Jul 2019 07:10:08 +0700
> This is for fixing bug KMSAN: uninit-value in ax88772_bind
>
> Tested by
> https://groups.google.com/d/msg/syzkaller-bugs/aFQurGotng4/eB_HlNhhCwAJ
>
> Reported-by: syzbot+8a3fc6674bbc3978ed4e@syzkaller.appspotmail.com
>
> syzbot found the following crash on:
>
> HEAD commit: f75e4cfe kmsan: use kmsan_handle_urb() in urb.c
> git tree: kmsan
...
> Signed-off-by: Phong Tran <tranmanphong@gmail.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next 0/2] Mellanox, mlx5 devlink versions query
From: Jakub Kicinski @ 2019-07-03 0:51 UTC (permalink / raw)
To: Saeed Mahameed; +Cc: David S. Miller, netdev@vger.kernel.org
In-Reply-To: <20190702235442.1925-1-saeedm@mellanox.com>
On Tue, 2 Jul 2019 23:55:07 +0000, Saeed Mahameed wrote:
> Hi Dave,
>
> This humble 2 patch series from Shay adds the support for devlink fw
> versions query to mlx5 driver.
>
> In the first patch we implement the needed fw commands to support this
> feature.
> In the 2nd patch we implement the devlink callbacks themselves.
>
> I am not sending this as a pull request since i am not sure when my next
> pull request is going to be ready, and these two patches are straight
> forward net-next patches.
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* Re: [PATCH net-next] mlxsw: spectrum_ptp: Fix validation in mlxsw_sp1_ptp_packet_finish()
From: David Miller @ 2019-07-02 22:32 UTC (permalink / raw)
To: petrm; +Cc: netdev, colin.king, idosch
In-Reply-To: <3f905fb4d20f266f777ef56648f7615edaaffc9c.1562094119.git.petrm@mellanox.com>
From: Petr Machata <petrm@mellanox.com>
Date: Tue, 2 Jul 2019 19:06:47 +0000
> Before mlxsw_sp1_ptp_packet_finish() sends the packet back, it validates
> whether the corresponding port is still valid. However the condition is
> incorrect: when mlxsw_sp_port == NULL, the code dereferences the port to
> compare it to skb->dev.
>
> The condition needs to check whether the port is present and skb->dev still
> refers to that port (or else is NULL). If that does not hold, bail out.
> Add a pair of parentheses to fix the condition.
>
> Fixes: d92e4e6e33c8 ("mlxsw: spectrum: PTP: Support timestamping on Spectrum-1")
> Reported-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Petr Machata <petrm@mellanox.com>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net 0/2] macsec: fix some bugs in the receive path
From: David Miller @ 2019-07-02 21:13 UTC (permalink / raw)
To: ast; +Cc: netdev, sd
In-Reply-To: <04d1ca9ff24de717746b5e21573656f6cb7069d6.camel@domdv.de>
From: Andreas Steinmetz <ast@domdv.de>
Date: Sun, 30 Jun 2019 22:46:38 +0200
> This series fixes some bugs in the receive path of macsec. The first
> is a use after free when processing macsec frames with a SecTAG that
> has the TCI E bit set but the C bit clear. In the 2nd bug, the driver
> leaves an invalid checksumming state after decrypting the packet.
>
> This is a combined effort of Sabrina Dubroca <sd@queasysnail.net> and me.
Series applied and queued up for -stable, thank you.
^ permalink raw reply
* Re: [PATCH V2] include: linux: Regularise the use of FIELD_SIZEOF macro
From: Kees Cook @ 2019-07-02 16:33 UTC (permalink / raw)
To: Joe Perches
Cc: Alexey Dobriyan, Andreas Dilger, Andrew Morton, Shyam Saini,
kernel-hardening, linux-kernel, linux-arm-kernel, linux-mips,
intel-gvt-dev, intel-gfx, dri-devel, netdev, linux-ext4, devel,
linux-mm, linux-sctp, bpf, kvm, mayhs11saini
In-Reply-To: <c3b83ba7f9b003dd4fb9cad885461ce93165dc04.camel@perches.com>
On Sat, Jun 29, 2019 at 09:45:10AM -0700, Joe Perches wrote:
> On Sat, 2019-06-29 at 17:25 +0300, Alexey Dobriyan wrote:
> > On Tue, Jun 11, 2019 at 03:00:10PM -0600, Andreas Dilger wrote:
> > > On Jun 11, 2019, at 2:48 PM, Andrew Morton <akpm@linux-foundation.org> wrote:
> > > > On Wed, 12 Jun 2019 01:08:36 +0530 Shyam Saini <shyam.saini@amarulasolutions.com> wrote:
> > > I did a check, and FIELD_SIZEOF() is used about 350x, while sizeof_field()
> > > is about 30x, and SIZEOF_FIELD() is only about 5x.
> > >
> > > That said, I'm much more in favour of "sizeof_field()" or "sizeof_member()"
> > > than FIELD_SIZEOF(). Not only does that better match "offsetof()", with
> > > which it is closely related, but is also closer to the original "sizeof()".
> > >
> > > Since this is a rather trivial change, it can be split into a number of
> > > patches to get approval/landing via subsystem maintainers, and there is no
> > > huge urgency to remove the original macros until the users are gone. It
> > > would make sense to remove SIZEOF_FIELD() and sizeof_field() quickly so
> > > they don't gain more users, and the remaining FIELD_SIZEOF() users can be
> > > whittled away as the patches come through the maintainer trees.
> >
> > The signature should be
> >
> > sizeof_member(T, m)
> >
> > it is proper English,
> > it is lowercase, so is easier to type,
> > it uses standard term (member, not field),
> > it blends in with standard "sizeof" operator,
>
> yes please.
>
> Also, a simple script conversion applied
> immediately after an rc1 might be easiest
> rather than individual patches.
This seems reasonable to me. I think the patch steps would be:
1) implement sizeof_member(T, m) as a stand-alone macro
2) do a scripted replacement of all identical macros.
3) remove all the identical macros.
Step 2 can be a patch that includes the script used to do the
replacement. That way Linus can choose to just run the script instead of
taking the patch.
--
Kees Cook
^ permalink raw reply
* Re: [PATCH net 0/6] gtp: fix several bugs
From: Harald Welte @ 2019-07-03 1:08 UTC (permalink / raw)
To: Taehee Yoo; +Cc: davem, pablo, Pau Espin, osmocom-net-gprs, netdev
In-Reply-To: <20190702152034.22412-1-ap420073@gmail.com>
Hi Taehee,
On Wed, Jul 03, 2019 at 12:20:34AM +0900, Taehee Yoo wrote:
> This patch series fixes several bugs in the gtp module.
thanks a lot for your patches, they are much appreciated.
They look valid to me after a brief initial review.
However, I'm currently on holidays and don't have the ability to test
any patches until my return on July 17. Maybe Pablo and/or Pau can have
a look meanwhile? Thanks in advance.
Regards,
Harald
--
- Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/
============================================================================
"Privacy in residential applications is a desirable marketing option."
(ETSI EN 300 175-7 Ch. A6)
^ permalink raw reply
* Re: [PATCH net] net: don't warn in inet diag when IPV6 is disabled
From: Stephen Hemminger @ 2019-07-02 22:14 UTC (permalink / raw)
To: David Miller; +Cc: kuznet, yoshfuji, netdev
In-Reply-To: <20190702.142347.1440800997923616328.davem@davemloft.net>
On Tue, 02 Jul 2019 14:23:47 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Mon, 1 Jul 2019 08:23:03 -0700
>
> > @@ -19,9 +19,11 @@ raw_get_hashinfo(const struct inet_diag_req_v2 *r)
> > {
> > if (r->sdiag_family == AF_INET) {
> > return &raw_v4_hashinfo;
> > -#if IS_ENABLED(CONFIG_IPV6)
> > } else if (r->sdiag_family == AF_INET6) {
> > +#if IS_ENABLED(CONFIG_IPV6)
> > return &raw_v6_hashinfo;
> > +#else
> > + return ERR_PTR(-EOPNOTSUPP);
> > #endif
> > } else {
> > pr_warn_once("Unexpected inet family %d\n",
>
> Let's make some consistency in this area please.
>
> The inet_diag code returns -EINVAL, and that's been that way forever.
> It also doesn't print a weird warning for unexpected sdiag_family
> values outside of AF_INET and AF_INET6.
>
> That's been that way for so long that's probably the behavior to
> revolve everything around.
>
> Therefore, please just get rid of the warning message instead of
> all of these other changes.
>
> Thank you.
Sure, that makes sense.
^ permalink raw reply
* Re: Validation of forward_delay seems wrong...
From: Nikolay Aleksandrov @ 2019-07-02 21:21 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Roopa Prabhu, bridge, netdev
In-Reply-To: <55f24bfb-4239-dda8-24f8-26b6b2fa9f9e@cumulusnetworks.com>
On 03/07/2019 00:19, Nikolay Aleksandrov wrote:
> On 02/07/2019 23:47, Andrew Lunn wrote:
>> Hi Nikolay
>>
>> The man page says that the bridge forward_delay is in units of
>> seconds, and should be between 2 and 30.
>>
>> I've tested on a couple of different kernel versions, and this appears
>> to be not working correctly:
>>
>> ip link set br0 type bridge forward_delay 2
>> RTNETLINK answers: Numerical result out of range
>>
>> ip link set br0 type bridge forward_delay 199
>> RTNETLINK answers: Numerical result out of range
>>
>> ip link set br0 type bridge forward_delay 200
>> #
>>
>> ip link set br0 type bridge forward_delay 3000
>> #
>>
>> ip link set br0 type bridge forward_delay 3001
>> RTNETLINK answers: Numerical result out of range
>>
>> I've not checked what delay is actually being used here, but clearly
>> something is mixed up.
>>
>> grep HZ .config
>> CONFIG_HZ_PERIODIC=y
>> # CONFIG_NO_HZ_IDLE is not set
>> # CONFIG_NO_HZ_FULL is not set
>> # CONFIG_NO_HZ is not set
>> CONFIG_HZ_FIXED=0
>> CONFIG_HZ_100=y
>> # CONFIG_HZ_200 is not set
>> # CONFIG_HZ_250 is not set
>> # CONFIG_HZ_300 is not set
>> # CONFIG_HZ_500 is not set
>> # CONFIG_HZ_1000 is not set
>> CONFIG_HZ=100
>>
>> Thanks
>> Andrew
>>
>
> Hi Andrew,
> The man page is wrong, these have been in USER_HZ scaled clock_t format from the beginning.
> TBH a lot of the time/delay bridge config options are messed up like that.
> We've been discussing adding special _ms versions in iproute2 to make them
> more user-friendly and understandable. Will cook a patch for the man page.
>
> Cheers,
> Nik
>
>
Err, I meant it is seconds just scaled, if it wasn't clear.
^ permalink raw reply
* Re: [PATCH net-next] tipc: remove ub->ubsock checks
From: David Miller @ 2019-07-02 22:13 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, jon.maloy, ying.xue, tipc-discussion
In-Reply-To: <d59889f395b2c224131046c832fe1a8056209107.1562000239.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Tue, 2 Jul 2019 00:57:19 +0800
> Both tipc_udp_enable and tipc_udp_disable are called under rtnl_lock,
> ub->ubsock could never be NULL in tipc_udp_disable and cleanup_bearer,
> so remove the check.
>
> Also remove the one in tipc_udp_enable by adding "free" label.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Looks good, applied.
^ permalink raw reply
* Re: [PATCH net-next] bonding/main: fix NULL dereference in bond_select_active_slave()
From: David Miller @ 2019-07-02 22:15 UTC (permalink / raw)
To: edumazet
Cc: netdev, eric.dumazet, jsperbeck, jarod, j.vosburgh, vfalico, andy
In-Reply-To: <20190701174851.70293-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Mon, 1 Jul 2019 10:48:51 -0700
> A bonding master can be up while best_slave is NULL.
...
> Fixes: e2a7420df2e0 ("bonding/main: convert to using slave printk macros")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: John Sperbeck <jsperbeck@google.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 6/9] libbpf: use negative fd to specify missing BTF
From: Stanislav Fomichev @ 2019-07-02 22:57 UTC (permalink / raw)
To: Andrii Nakryiko; +Cc: andrii.nakryiko, netdev, bpf, ast, daniel, kernel-team
In-Reply-To: <20190529173611.4012579-7-andriin@fb.com>
On 05/29, Andrii Nakryiko wrote:
> 0 is a valid FD, so it's better to initialize it to -1, as is done in
> other places. Also, technically, BTF type ID 0 is valid (it's a VOID
> type), so it's more reliable to check btf_fd, instead of
> btf_key_type_id, to determine if there is any BTF associated with a map.
>
> Acked-by: Song Liu <songliubraving@fb.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/libbpf.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index c972fa10271f..a27a0351e595 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1751,7 +1751,7 @@ bpf_object__create_maps(struct bpf_object *obj)
> create_attr.key_size = def->key_size;
> create_attr.value_size = def->value_size;
> create_attr.max_entries = def->max_entries;
> - create_attr.btf_fd = 0;
> + create_attr.btf_fd = -1;
> create_attr.btf_key_type_id = 0;
> create_attr.btf_value_type_id = 0;
> if (bpf_map_type__is_map_in_map(def->type) &&
> @@ -1765,11 +1765,11 @@ bpf_object__create_maps(struct bpf_object *obj)
> }
>
> *pfd = bpf_create_map_xattr(&create_attr);
> - if (*pfd < 0 && create_attr.btf_key_type_id) {
> + if (*pfd < 0 && create_attr.btf_fd >= 0) {
> cp = libbpf_strerror_r(errno, errmsg, sizeof(errmsg));
> pr_warning("Error in bpf_create_map_xattr(%s):%s(%d). Retrying without BTF.\n",
> map->name, cp, errno);
> - create_attr.btf_fd = 0;
> + create_attr.btf_fd = -1;
This breaks libbpf compatibility with the older kernels. If the kernel
doesn't know about btf_fd and we set it to -1, then CHECK_ATTR
fails :-(
Any objections to converting BTF retries to bpf_capabilities and then
knowingly passing bft_fd==0 or proper fd?
> create_attr.btf_key_type_id = 0;
> create_attr.btf_value_type_id = 0;
> map->btf_key_type_id = 0;
> @@ -2053,6 +2053,9 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> char *log_buf;
> int ret;
>
> + if (!insns || !insns_cnt)
> + return -EINVAL;
> +
> memset(&load_attr, 0, sizeof(struct bpf_load_program_attr));
> load_attr.prog_type = prog->type;
> load_attr.expected_attach_type = prog->expected_attach_type;
> @@ -2063,7 +2066,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> load_attr.license = license;
> load_attr.kern_version = kern_version;
> load_attr.prog_ifindex = prog->prog_ifindex;
> - load_attr.prog_btf_fd = prog->btf_fd >= 0 ? prog->btf_fd : 0;
> + load_attr.prog_btf_fd = prog->btf_fd;
> load_attr.func_info = prog->func_info;
> load_attr.func_info_rec_size = prog->func_info_rec_size;
> load_attr.func_info_cnt = prog->func_info_cnt;
> @@ -2072,8 +2075,6 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> load_attr.line_info_cnt = prog->line_info_cnt;
> load_attr.log_level = prog->log_level;
> load_attr.prog_flags = prog->prog_flags;
> - if (!load_attr.insns || !load_attr.insns_cnt)
> - return -EINVAL;
>
> retry_load:
> log_buf = malloc(log_buf_size);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC iproute2] netns: add mounting state file for each netns
From: Stephen Hemminger @ 2019-07-03 0:41 UTC (permalink / raw)
To: Matteo Croce; +Cc: netdev, Alexander Aring, Nicolas Dichtel
In-Reply-To: <20190630192933.30743-1-mcroce@redhat.com>
On Sun, 30 Jun 2019 21:29:33 +0200
Matteo Croce <mcroce@redhat.com> wrote:
> @@ -737,6 +746,14 @@ static int netns_add(int argc, char **argv, bool create)
> }
> close(fd);
>
> + fd = open(tmp_path, O_RDONLY|O_CREAT|O_EXCL, 0);
> + if (fd < 0) {
> + fprintf(stderr, "Cannot create namespace file \"%s\": %s\n",
> + tmp_path, strerror(errno));
> + goto out_delete;
> + }
> + close(fd);
> +
> if (create) {
> netns_save();
> if (unshare(CLONE_NEWNET) < 0) {
> @@ -757,6 +774,7 @@ static int netns_add(int argc, char **argv, bool create)
> goto out_delete;
> }
> netns_restore();
> + unlink(tmp_path);
This looks like yet another source of potential errors and races.
What if the program is killed or other issues.
Maybe using abstract unix domain socket (which doesn't exist in filesystem
and auto-deletes on exit) would be better.
^ permalink raw reply
* Re: [PATCH] ipv4: Fix off-by-one in route dump counter without netlink strict checking
From: David Miller @ 2019-07-02 21:07 UTC (permalink / raw)
To: sbrivio; +Cc: dsahern, netdev
In-Reply-To: <74faa085e6af026f8b9f0d3cce8a94147781f257.1561830851.git.sbrivio@redhat.com>
From: Stefano Brivio <sbrivio@redhat.com>
Date: Sat, 29 Jun 2019 19:55:08 +0200
> In commit ee28906fd7a1 ("ipv4: Dump route exceptions if requested") I
> added a counter of per-node dumped routes (including actual routes and
> exceptions), analogous to the existing counter for dumped nodes. Dumping
> exceptions means we need to also keep track of how many routes are dumped
> for each node: this would be just one route per node, without exceptions.
>
> When netlink strict checking is not enabled, we dump both routes and
> exceptions at the same time: the RTM_F_CLONED flag is not used as a
> filter. In this case, the per-node counter 'i_fa' is incremented by one
> to track the single dumped route, then also incremented by one for each
> exception dumped, and then stored as netlink callback argument as skip
> counter, 's_fa', to be used when a partial dump operation restarts.
>
> The per-node counter needs to be increased by one also when we skip a
> route (exception) due to a previous non-zero skip counter, because it
> needs to match the existing skip counter, if we are dumping both routes
> and exceptions. I missed this, and only incremented the counter, for
> regular routes, if the previous skip counter was zero. This means that,
> in case of a mixed dump, partial dump operations after the first one
> will start with a mismatching skip counter value, one less than expected.
>
> This means in turn that the first exception for a given node is skipped
> every time a partial dump operation restarts, if netlink strict checking
> is not enabled (iproute < 5.0).
>
> It turns out I didn't repeat the test in its final version, commit
> de755a85130e ("selftests: pmtu: Introduce list_flush_ipv4_exception test
> case"), which also counts the number of route exceptions returned, with
> iproute2 versions < 5.0 -- I was instead using the equivalent of the IPv6
> test as it was before commit b964641e9925 ("selftests: pmtu: Make
> list_flush_ipv6_exception test more demanding").
>
> Always increment the per-node counter by one if we previously dumped
> a regular route, so that it matches the current skip counter.
>
> Fixes: ee28906fd7a1 ("ipv4: Dump route exceptions if requested")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
Applied to net-next, thanks for fixing this.
^ permalink raw reply
* Re: [PATCH net-next] af_packet: convert pending frame counter to atomic_t
From: David Miller @ 2019-07-02 21:03 UTC (permalink / raw)
To: nhorman; +Cc: netdev, willemb
In-Reply-To: <20190628145206.13871-1-nhorman@tuxdriver.com>
From: Neil Horman <nhorman@tuxdriver.com>
Date: Fri, 28 Jun 2019 10:52:06 -0400
> Given that the socket transmit path is an exclusive path (locked via the
> pg_vec_lock mutex), we do not have the ability to increment this counter
> on multiple cpus in parallel. This implementation also seems to have
> the potential to be broken, in that, should an skb be freed on a cpu
> other than the one that it was initially transmitted on, we may
> decrement a counter that was not initially incremented, leading to
> underflow.
This isn't a problem. There is only one valid "read" operation and that
is the "summation" of all of the per-cpu counters.
All of the overflows and underflows cancel out in the situation you
describe, so all is fine.
I'm hesitant to get behind any fiddling in this area.
Thanks Neil.
^ permalink raw reply
* Re: [PATCH net-next] tipc: use rcu dereference functions properly
From: David Miller @ 2019-07-02 22:08 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, jon.maloy, ying.xue, tipc-discussion
In-Reply-To: <07e0518ac689f5919890a38634df38edf95d34a1.1562000095.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Tue, 2 Jul 2019 00:54:55 +0800
> For these places are protected by rcu_read_lock, we change from
> rcu_dereference_rtnl to rcu_dereference, as there is no need to
> check if rtnl lock is held.
>
> For these places are protected by rtnl_lock, we change from
> rcu_dereference_rtnl to rtnl_dereference/rcu_dereference_protected,
> as no extra memory barriers are needed under rtnl_lock() which also
> protects tn->bearer_list[] and dev->tipc_ptr/b->media_ptr updating.
>
> rcu_dereference_rtnl will be only used in the places where it could
> be under rcu_read_lock or rtnl_lock.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
In the cases where RTNL is held, even if rcu_read_lock() is also taken,
we should use rtnl_dereference() because that avoids the READ_ONCE().
^ permalink raw reply
* Re: [PATCH] net: ethernet: mediatek: Allow non TRGMII mode with MT7621 DDR2 devices
From: David Miller @ 2019-07-02 21:06 UTC (permalink / raw)
To: opensource
Cc: sean.wang, f.fainelli, linux, matthias.bgg, andrew,
vivien.didelot, frank-w, netdev, linux-mediatek, linux-mips
In-Reply-To: <20190629122451.19578-1-opensource@vdorst.com>
From: René van Dorst <opensource@vdorst.com>
Date: Sat, 29 Jun 2019 14:24:51 +0200
> No reason to error out on a MT7621 device with DDR2 memory when non
> TRGMII mode is selected.
> Only MT7621 DDR2 clock setup is not supported for TRGMII mode.
> But non TRGMII mode doesn't need any special clock setup.
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH bpf] xdp: fix race on generic receive path
From: Jakub Kicinski @ 2019-07-03 0:40 UTC (permalink / raw)
To: Ilya Maximets
Cc: netdev, linux-kernel, bpf, xdp-newbies, David S. Miller,
Björn Töpel, Magnus Karlsson, Jonathan Lemon,
Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <20190702143634.19688-1-i.maximets@samsung.com>
On Tue, 2 Jul 2019 17:36:34 +0300, Ilya Maximets wrote:
> Unlike driver mode, generic xdp receive could be triggered
> by different threads on different CPU cores at the same time
> leading to the fill and rx queue breakage. For example, this
> could happen while sending packets from two processes to the
> first interface of veth pair while the second part of it is
> open with AF_XDP socket.
>
> Need to take a lock for each generic receive to avoid race.
>
> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a14e8864e4fa..19f41d2b670c 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -119,17 +119,22 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
> {
> u32 metalen = xdp->data - xdp->data_meta;
> u32 len = xdp->data_end - xdp->data;
> + unsigned long flags;
> void *buffer;
> u64 addr;
> int err;
>
> - if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
> - return -EINVAL;
> + spin_lock_irqsave(&xs->rx_lock, flags);
Why _irqsave, rather than _bh?
> + if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
> + err = -EINVAL;
> + goto out_unlock;
> + }
>
> if (!xskq_peek_addr(xs->umem->fq, &addr) ||
> len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
> - xs->rx_dropped++;
> - return -ENOSPC;
> + err = -ENOSPC;
> + goto out_drop;
> }
>
> addr += xs->umem->headroom;
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 21:02 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap, brouer
In-Reply-To: <20190702185839.GH4510@khorivan>
On Tue, 2 Jul 2019 21:58:40 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Tue, Jul 02, 2019 at 08:29:07PM +0200, Jesper Dangaard Brouer wrote:
> >On Tue, 2 Jul 2019 18:21:13 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> On Tue, Jul 02, 2019 at 05:10:29PM +0200, Jesper Dangaard Brouer wrote:
> >> >On Tue, 2 Jul 2019 17:56:13 +0300
> >> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >> >
> >> >> On Tue, Jul 02, 2019 at 04:52:30PM +0200, Jesper Dangaard Brouer wrote:
> >> >> >On Tue, 2 Jul 2019 17:44:27 +0300
> >> >> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >> >> >
> >> >> >> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
> >> >> >> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> >> >
> >> >> >> >Jesper recently removed page_pool_destroy() (from driver invocation) and
> >> >> >> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
> >> >> >> >handle in-flight packets/pages. This created an asymmetry in drivers
> >> >> >> >create/destroy pairs.
> >> >> >> >
> >> >> >> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
> >> >> >> >This serves two purposes, (1) simplify drivers error handling as driver now
> >> >> >> >drivers always calls page_pool_destroy() and don't need to track if
> >> >> >> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
> >> >> >> >where a single RX-queue (with a single page_pool) provides packets for two
> >> >> >> >net_device'es, and thus needs to register the same page_pool twice with two
> >> >> >> >xdp_rxq_info structures.
> >> >> >>
> >> >> >> As I tend to use xdp level patch there is no more reason to mention (2) case
> >> >> >> here. XDP patch serves it better and can prevent not only obj deletion but also
> >> >> >> pool flush, so, this one patch I could better leave only for (1) case.
> >> >> >
> >> >> >I don't understand what you are saying.
> >> >> >
> >> >> >Do you approve this patch, or do you reject this patch?
> >> >> >
> >> >> It's not reject, it's proposition to use both, XDP and page pool patches,
> >> >> each having its goal.
> >> >
> >> >Just to be clear, if you want this patch to get accepted you have to
> >> >reply with your Signed-off-by (as I wrote).
> >> >
> >> >Maybe we should discuss it in another thread, about why you want two
> >> >solutions to the same problem.
> >>
> >> If it solves same problem I propose to reject this one and use this:
> >> https://lkml.org/lkml/2019/7/2/651
> >
> >No, I propose using this one, and rejecting the other one.
>
> There is at least several arguments against this one (related (2) purpose)
>
> It allows:
> - avoid changes to page_pool/mlx5/netsec
> - save not only allocator obj but allocator "page/buffer flush"
> - buffer flush can be present not only in page_pool but for other allocators
> that can behave differently and not so simple solution.
> - to not limit cpsw/(potentially others) to use "page_pool" allocator only
> ....
>
> This patch better leave also, as it simplifies error path for page_pool and
> have more error prone usage comparing with existent one.
>
> Please, don't limit cpsw and potentially other drivers to use only
> page_pool it can be zca or etc... I don't won't to modify each allocator.
> I propose to add both as by fact they solve different problems with common
> solution.
I'm trying to limit the scope of your changes, for your special case,
because I'm afraid this more common solution is going to limit our
options, painting ourselves into a corner.
E.g. for correct lifetime handling, I think we actually need to do a
dev_hold() on the net_device. (Changes in f71fec47c2 might not be
enough, but I first need to dig into the details and ask Hellwig about
some details). Adding that after your patch is more complicated (if
even doable).
E.g. doing dev_hold() on the net_device, can also turn into a
performance advantage, when/if page_pool is extended to also "travel"
into SKBs. (Allowing to elide such dev_hold() calls in netstack).
I also worry about the possible performance impact these changes will
have down the road. (For the RX/alloc side it should be clear by now
that we gain a lot of performance with the single RX-queue binding and
napi protection). On the return/free side performance *need* to be
improved (it doesn't scale). I'm basically looking at different ways
to bulk return pages into the ptr_ring, which requires changes in
page_pool and likely in xdp_allocator structure. Which your changes
are complicating.
This special use-case, seems confined to your driver. And Ilias told me
that XDP is not really a performance benefit for this driver as the HW
PPS-limit is hit before the XDP and netstack limit. I ask, does it
make sense to add XDP to this driver, if it complicates the code for
everybody else?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Jakub Kicinski @ 2019-07-02 23:42 UTC (permalink / raw)
To: Parav Pandit; +Cc: Jiri Pirko, netdev@vger.kernel.org, Saeed Mahameed
In-Reply-To: <AM0PR05MB4866C19C9E6ED767A44C3064D1F80@AM0PR05MB4866.eurprd05.prod.outlook.com>
On Tue, 2 Jul 2019 18:50:31 +0000, Parav Pandit wrote:
> > > I didn't see any immediate need to report, at the same time didn't
> > > find any reason to treat such port flavours differently than existing
> > > one. It just gives a clear view of the device's eswitch. Might find it
> > > useful during debugging while inspecting device internal tables..
> >
> > PFs and VFs ports are not tied to network ports in switchdev mode.
> > You have only one network port under a devlink instance AFAIR, anyway.
> >
> I am not sure what do you mean by network port.
DEVLINK_PORT_FLAVOUR_PHYSICAL
> Do you intent to see a physical port that connects to physical network?
>
> As I described in the comment of the PF and VF flavour, it is an eswitch port.
> I have shown the diagram also of the eswitch in the cover letter.
> Port_number doesn't have to a physical port. Flavour describe what
> port type is and number says what is the eswitch port number.
> Hope it clarifies.
I understand what you're doing. If you want to expose some device
specific eswitch port ID please add a new attribute for that.
The fact that that ID may match port_number for your device today
is coincidental. port_number, and split attributes should not be
exposed for PCI ports.
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 21:41 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: Jesper Dangaard Brouer, netdev, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <20190702211536.GA22618@apalos>
On Wed, Jul 03, 2019 at 12:15:36AM +0300, Ilias Apalodimas wrote:
>Hi Jesper,
>Getting late here, i'll respond in detail tomorrow. One point though
>
>[...]
>>
>> This special use-case, seems confined to your driver. And Ilias told me
>> that XDP is not really a performance benefit for this driver as the HW
>> PPS-limit is hit before the XDP and netstack limit. I ask, does it
>> make sense to add XDP to this driver, if it complicates the code for
>> everybody else?
>I think yes. This is a widely used driver on TI embedded devices so having XDP
>to play along is a nice feature. It's also the first and only armv7 we have
>supporting this. Ivan already found a couple of issues due to the 32-bit
>architecture he is trying to fix, i think there's real benefit in having that,
>performance aside.
>I fully agree we should not impact the performance of the API to support a
>special hardware though. I'll have a look on the 2 solutions tomorrow, but the
>general approach on this one should be 'the simpler the better'
>
>Cheers
>/Ilias
BTW even w/o optimization it has close to 300kpps (but with increased number of
descs) on drop which is very close to netsec measurements Ilias sent. But from
what I know there is no h/w limit on cpsw at all that this CPU can serve, so
my assumption it's rather s/w limit. But that's not main here and XDP usage
has not been estimated enough yet in embedded, where hi speed not only benefit
that can be taken from XDP.
I need more clear circumstances to send v6 ...
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net] net/ibmvnic: Report last valid speed and duplex values to ethtool
From: David Miller @ 2019-07-02 21:01 UTC (permalink / raw)
To: tlfalcon; +Cc: netdev, linuxppc-dev, bjking1, pradeep, dnbanerg
In-Reply-To: <1561655353-17114-1-git-send-email-tlfalcon@linux.ibm.com>
From: Thomas Falcon <tlfalcon@linux.ibm.com>
Date: Thu, 27 Jun 2019 12:09:13 -0500
> This patch resolves an issue with sensitive bonding modes
> that require valid speed and duplex settings to function
> properly. Currently, the adapter will report that device
> speed and duplex is unknown if the communication link
> with firmware is unavailable. This decision can break LACP
> configurations if the timing is right.
>
> For example, if invalid speeds are reported, the slave
> device's link state is set to a transitional "fail" state
> and the LACP port is disabled. However, if valid speeds
> are reported later but the link state has not been altered,
> the LACP port will remain disabled. If the link state then
> transitions back to "up" from "fail," it results in a state
> such that the slave reports valid speed/duplex and is up,
> but the LACP port will remain disabled.
>
> Workaround this by reporting the last recorded speed
> and duplex settings unless the device has never been
> activated. In that case or when the hypervisor gives
> invalid values, continue to report unknown speed or
> duplex to ethtool.
>
> Signed-off-by: Thomas Falcon <tlfalcon@linux.ibm.com>
Like Andrew, I have my conerns about this.
If the firmware is unavailable, the link is effectively down. So
you should report link down and unknown link parameters.
Bonding and LACP should do the right thing when the firwmare is
reachable again after the migration and the link goes back up.
If bonding/LACP isn't doing that, then the bug is there.
^ permalink raw reply
* Re: [PATCHv2] selftests/net: skip psock_tpacket test if KALLSYMS was not enabled
From: David Miller @ 2019-07-03 0:18 UTC (permalink / raw)
To: po-hsu.lin; +Cc: shuah, linux-kselftest, netdev, linux-kernel
In-Reply-To: <20190701044031.19451-1-po-hsu.lin@canonical.com>
From: Po-Hsu Lin <po-hsu.lin@canonical.com>
Date: Mon, 1 Jul 2019 12:40:31 +0800
> The psock_tpacket test will need to access /proc/kallsyms, this would
> require the kernel config CONFIG_KALLSYMS to be enabled first.
>
> Apart from adding CONFIG_KALLSYMS to the net/config file here, check the
> file existence to determine if we can run this test will be helpful to
> avoid a false-positive test result when testing it directly with the
> following commad against a kernel that have CONFIG_KALLSYMS disabled:
> make -C tools/testing/selftests TARGETS=net run_tests
>
> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: santosh.shilimkar @ 2019-07-02 22:47 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <74255414-7e5c-e490-4745-9a8b9a73488d@oracle.com>
On 7/2/19 3:12 PM, Gerd Rausch wrote:
> On 02/07/2019 14.18, santosh.shilimkar@oracle.com wrote:
>> On 7/2/19 2:05 PM, Gerd Rausch wrote:
>>> What do you call "RDS_GET_MR" semantics?
>>>
>> Its a blocking socket call. Meaning after this call return to the
>> user, the key must be valid. With async registration that can't be
>> guaranteed.
>>
>
> If the "IB_WR_REG_MR" operation does not complete successfully within
> the given (to-be-discussed?) timeout, "rds_ib_post_reg_frmr" will return
> "-EBUSY".
>
> And that should propagate up the entire stack and make its way into
> "setsockopt" returning "-1" with "errno == EBUSY".
This is an easy case and this doesn't need any waiting since call just
came back without posting work request.
>
> Do you see a problem with this approach?
> Did you observe a situation where this did not work?
>
Calling rds_ib_post_reg_frmr() and looking at return value doesn't
grantee that the work request postred is gping to be successful.
> Are you saying that no timeout, no matter how large, is large enough?
> If that's the case, we can consider turning the "wait_event_timeout"
> into a "wait_event".
>
Yep. Basically till the ceq handler reports successful completion of
reg_mr or inval_mr, mr is not guaranteed to be registered or
invalidated.
>>> Are you suggesting to
>>> a) Not fix this bug right now and wait until some later point in time
>> When did I say that ? I said have you explored alternate approach to
>> fix the issue and if not could you try it out.
>>
>
> Why explore an alternate approach?
> Do you see a problem with the proposed patch (other than the choice of timeout)?
>
Yes the timeout based proceeding isn't safe. wait_event without timeout
would make it guaranteed and give sync like behavior. This is the
behavior with FMR reg and inval calls. If you make that as
wait_event then am fine with the change.
Regards,
Santosh
^ permalink raw reply
* [PATCH net-next v5 2/5] net: core: move pop MPLS functionality from OvS to core helper
From: John Hurley @ 2019-07-03 0:25 UTC (permalink / raw)
To: netdev
Cc: davem, jiri, xiyou.wangcong, dsahern, willemdebruijn.kernel,
simon.horman, jakub.kicinski, oss-drivers, John Hurley
In-Reply-To: <1562113531-29296-1-git-send-email-john.hurley@netronome.com>
Open vSwitch provides code to pop an MPLS header to a packet. In
preparation for supporting this in TC, move the pop code to an skb helper
that can be reused.
Remove the, now unused, update_ethertype static function from OvS.
Signed-off-by: John Hurley <john.hurley@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
include/linux/skbuff.h | 1 +
net/core/skbuff.c | 42 ++++++++++++++++++++++++++++++++++++++++++
net/openvswitch/actions.c | 37 ++-----------------------------------
3 files changed, 45 insertions(+), 35 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 0112256..89d5c43 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3447,6 +3447,7 @@ int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci);
int skb_vlan_pop(struct sk_buff *skb);
int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto);
+int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto);
struct sk_buff *pskb_extract(struct sk_buff *skb, int off, int to_copy,
gfp_t gfp);
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f1d1e47..ce30989 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -5391,6 +5391,48 @@ int skb_mpls_push(struct sk_buff *skb, __be32 mpls_lse, __be16 mpls_proto)
EXPORT_SYMBOL_GPL(skb_mpls_push);
/**
+ * skb_mpls_pop() - pop the outermost MPLS header
+ *
+ * @skb: buffer
+ * @next_proto: ethertype of header after popped MPLS header
+ *
+ * Expects skb->data at mac header.
+ *
+ * Returns 0 on success, -errno otherwise.
+ */
+int skb_mpls_pop(struct sk_buff *skb, __be16 next_proto)
+{
+ int err;
+
+ if (unlikely(!eth_p_mpls(skb->protocol)))
+ return -EINVAL;
+
+ err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
+ if (unlikely(err))
+ return err;
+
+ skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
+ memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
+ skb->mac_len);
+
+ __skb_pull(skb, MPLS_HLEN);
+ skb_reset_mac_header(skb);
+ skb_set_network_header(skb, skb->mac_len);
+
+ if (skb->dev && skb->dev->type == ARPHRD_ETHER) {
+ struct ethhdr *hdr;
+
+ /* use mpls_hdr() to get ethertype to account for VLANs. */
+ hdr = (struct ethhdr *)((void *)mpls_hdr(skb) - ETH_HLEN);
+ skb_mod_eth_type(skb, hdr, next_proto);
+ }
+ skb->protocol = next_proto;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(skb_mpls_pop);
+
+/**
* alloc_skb_with_frags - allocate skb with page frags
*
* @header_len: size of linear part
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index a9a6c9c..62715bb 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -160,18 +160,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
struct sw_flow_key *key,
const struct nlattr *attr, int len);
-static void update_ethertype(struct sk_buff *skb, struct ethhdr *hdr,
- __be16 ethertype)
-{
- if (skb->ip_summed == CHECKSUM_COMPLETE) {
- __be16 diff[] = { ~(hdr->h_proto), ethertype };
-
- skb->csum = csum_partial((char *)diff, sizeof(diff), skb->csum);
- }
-
- hdr->h_proto = ethertype;
-}
-
static int push_mpls(struct sk_buff *skb, struct sw_flow_key *key,
const struct ovs_action_push_mpls *mpls)
{
@@ -190,31 +178,10 @@ static int pop_mpls(struct sk_buff *skb, struct sw_flow_key *key,
{
int err;
- err = skb_ensure_writable(skb, skb->mac_len + MPLS_HLEN);
- if (unlikely(err))
+ err = skb_mpls_pop(skb, ethertype);
+ if (err)
return err;
- skb_postpull_rcsum(skb, mpls_hdr(skb), MPLS_HLEN);
-
- memmove(skb_mac_header(skb) + MPLS_HLEN, skb_mac_header(skb),
- skb->mac_len);
-
- __skb_pull(skb, MPLS_HLEN);
- skb_reset_mac_header(skb);
- skb_set_network_header(skb, skb->mac_len);
-
- if (ovs_key_mac_proto(key) == MAC_PROTO_ETHERNET) {
- struct ethhdr *hdr;
-
- /* mpls_hdr() is used to locate the ethertype field correctly in the
- * presence of VLAN tags.
- */
- hdr = (struct ethhdr *)((void *)mpls_hdr(skb) - ETH_HLEN);
- update_ethertype(skb, hdr, ethertype);
- }
- if (eth_p_mpls(skb->protocol))
- skb->protocol = ethertype;
-
invalidate_flow_key(key);
return 0;
}
--
2.7.4
^ permalink raw reply related
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