[4.4] Security fixes (pinctrl, i40e, geneve)
Here are backports of some fixes to the 4.4 stable branch. I wasn't able to test the pinctrl fix (no idea how to reproduce it). I wasn't able to test the i40e changes (no hardware and no reproducer available). I tested the geneve fix with libreswan as (roughly) described in the commit message. Ben. -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom security-4.4.mbox Description: application/mbox
[4.9] Security fixes (pinctrl, i40e, geneve)
Here are backports of some fixes to the 4.9 stable branch. I wasn't able to test the pinctrl fix (no idea how to reproduce it). I wasn't able to test the i40e changes (no hardware and no reproducer available). I tested the geneve fix with libreswan as (roughly) described in the commit message. Ben. -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom security-4.9.mbox Description: application/mbox
Re: Bug#966459: linux: traffic class socket options (both IPv4/IPv6) inconsistent with docs/standards
On Sun, 2020-08-02 at 22:44 +0200, Thorsten Glaser wrote: > On Sun, 2 Aug 2020, Ben Hutchings wrote: > > > The RFC says that the IPV6_TCLASS option's value is an int, and that > > for setsockopt (“option's”), not cmsg > > > No, the wording is *not* clear. > > Agreed. > > So perhaps let’s try to find out what’s actually right… For what it's worth, FreeBSD/Darwin and Windows also put 4 bytes of data in a IPV6_TCLASS cmsg. So whether or not it's "right", it's consistent between three independent implementations. Ben. -- Ben Hutchings For every complex problem there is a solution that is simple, neat, and wrong. signature.asc Description: This is a digitally signed message part
Re: Bug#966459: linux: traffic class socket options (both IPv4/IPv6) inconsistent with docs/standards
On Sun, 2020-08-02 at 19:29 +, Thorsten Glaser wrote: > Ben Hutchings dixit: > > >ip(7) also doesn't document IP_PKTOPIONS. > > Hmm, I don’t use IP_PKTOPIONS though. I’m not exactly sure I found > the correct place in the kernel for what I do. The first instance of put_cmsg(...IP_TOS...) you found in net/ipv4/ip_sockglue.c implements that socket option. [...] > >I see no point in changing the IPv6 behaviour: it seems to be > >consistent with itself and with the standard > > Not really: if the kernel writes an int and userspace reads > its first byte, it only works by accident on little endian, > but not elsewhere. The RFC says that the IPV6_TCLASS option's value is an int, and that "the first byte of cmsg_data[] will be the *first byte of the integer* traffic class" (my emphasis). We can infer from "the first byte of" that cmsg_data[] will hold more than one byte. And "the integer" suggests that it's a C int, like the socket option. > >so only risks breaking user-space that works today. > > Hrm. It risks breaking userspace that reads an int. But the > RFC clearly says it should read the first byte, not an int. [...] No, the wording is *not* clear. Ben. -- Ben Hutchings It is easier to write an incorrect program than to understand a correct one. signature.asc Description: This is a digitally signed message part
Re: Bug#966459: linux: traffic class socket options (both IPv4/IPv6) inconsistent with docs/standards
[The previous message is archived at <https://bugs.debian.org/966459>.] On Tue, 2020-07-28 at 20:31 +0200, Thorsten Glaser wrote: > Package: src:linux > Version: 5.7.6-1 > Severity: normal > Tags: upstream > X-Debbugs-Cc: t...@mirbsd.de > > I’m using setsockopt to set the traffic class on sending and receive > it in control messages on receiving, for both IPv4 and IPv6. > > The relevant documentation is the ip(7) manpage and, because the ipv6(7) > manpage doesn’t contain it, RFC3542. ip(7) also doesn't document IP_PKTOPIONS. [...] > Same in net/ipv4/ip_sockglue.c… > > int tos = inet->rcv_tos; > put_cmsg(&msg, SOL_IP, IP_TOS, sizeof(tos), &tos); > … in one place, but… > > put_cmsg(msg, SOL_IP, IP_TOS, 1, &ip_hdr(skb)->tos); > > … in ip_cmsg_recv_tos(), yielding inconsistent results for IPv4(!). Those are two different APIs though: recvmsg() for datagram sockets, vs getsockopt(... IP_PKTOPTIONS ...) for stream sockets. They obviously ought to be consistent, but mistakes happen. [...] > tl;dr: Receiving traffic class values from IP traffic is broken on > big endian platforms. Some user-space that uses getsockopt(... IP_PKTOPTIONS ...) for stream sockets might be broken. I searched for 'cmsg_type.*IP_TOS' on codesearch.debian.net, and found only two instances where it was used in conjunction with IP_PKTOPTIONS. libzorpll reads only the first byte (so is broken on big-endian): https://sources.debian.org/src/libzorpll/7.0.1.0%7Ealpha1-1.1/src/io.cc/#L239 squid reads an int and then truncates it to a byte (so is fine): https://sources.debian.org/src/squid/4.12-1/src/ip/QosConfig.cc/#L41 > I place the following suggestion for discussion, to achieve maximum > portability: put 4 bytes into the CMSG for both IPv4 and IPv6, where > the first and fourth byte are, identically, traffic class, second and > third 0. [...] I see no point in changing the IPv6 behaviour: it seems to be consistent with itself and with the standard, so only risks breaking user-space that works today. As for IPv4, changing the format of the IP_TOS field in the IP_PKTOPIONS value looks like it would work for the two users found in Debian. But you should know that the highest priority for Linux API compatibility is to avoid breaking currently working user-space. That means that ugly and inconsistent APIs won't get fixed if it causes a regression for the programs people actually use. If the API never worked like it was supposed to on some architectures, that's not a regression, and is lower priority. Ben. -- Ben Hutchings It is easier to write an incorrect program than to understand a correct one. signature.asc Description: This is a digitally signed message part
Re: [PATCH net] mlx4: Fix information leak on failure to read module EEPROM
On Mon, 2020-05-18 at 16:47 +, Saeed Mahameed wrote:
> On Sun, 2020-05-17 at 18:20 +0100, Ben Hutchings wrote:
> > mlx4_en_get_module_eeprom() returns 0 even if it fails. This results
> > in copying an uninitialised (or partly initialised) buffer back to
> > user-space.
[...]
> I am not sure i see the issue in here, and why we need the partial
> memset ?
> first thing in this function we do:
> memset(data, 0, ee->len);
>
> and then mlx4_get_module_info() will only copy valid data only on
> success.
Wow, sorry, I don't know how I missed that. So this is not the bug I
was looking for.
>
> > - if (!ret) /* Done reading */
> > + if (!ret) {
> > + /* DOM was not readable after all */
>
> actually if mlx4_get_module_info() returns any non-negative value it
> means how much data was read, so if it returns 0, it means that this
> was the last iteration and we are done reading the eeprom..
>
> so i would remove the above comment and the memset below is redundant
> since we already memset the whole buffer before the while loop.
Right.
> > + memset(data + i, 0, ee->len - i);
> > return 0;
> > + }
> >
> > if (ret < 0) {
> > en_err(priv,
> >"mlx4_get_module_info i(%d) offset(%d)
> > bytes_to_read(%d) - FAILED (0x%x)\n",
> >i, offset, ee->len - i, ret);
> > - return 0;
> > + return ret;
>
> I think returning error in here was the actual solution for your
> problem. you can verify by looking in the kernel log and verify you see
> the log message.
The original bug report (https://bugs.debian.org/960702) says that
ethtool reports different values depending on whether its output is
redirected. Although returning all-zeroes for the unreadable part
might be wrong, it doesn't explain that behaviour.
Perhaps if the timing of the I²C reads is marginal, varying numbers of
bytes of DOM information might be readable? But I don't see how
redirection of ethtool's output would affect that. It uses a single
ioctl to read everything, and the kernel controls timing within that.
So I am mystified about what is going on here. Maybe there is a bug in
ethtool, but I'm not seeing it.
Ben.
> > }
> >
> > i += ret;
--
Ben Hutchings
The two most common things in the universe are hydrogen and stupidity.
signature.asc
Description: This is a digitally signed message part
[PATCH net] mlx4: Fix information leak on failure to read module EEPROM
mlx4_en_get_module_eeprom() returns 0 even if it fails. This results
in copying an uninitialised (or partly initialised) buffer back to
user-space.
Change it so that:
* In the special case that the DOM turns out not to be readable, the
remaining part of the buffer is cleared. This should avoid a
regression when reading modules with this problem.
* In other error cases, the error code is propagated.
Reported-by: Yannis Aribaud
References: https://bugs.debian.org/960702
Fixes: 7202da8b7f71 ("ethtool, net/mlx4_en: Cable info, get_module_info/...")
Signed-off-by: Ben Hutchings
---
This is compile-tested only. It should go to stable, if it is a
correct fix.
Ben.
drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
index 8a5ea2543670..6edc3177af1c 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c
@@ -2078,14 +2078,17 @@ static int mlx4_en_get_module_eeprom(struct net_device
*dev,
ret = mlx4_get_module_info(mdev->dev, priv->port,
offset, ee->len - i, data + i);
- if (!ret) /* Done reading */
+ if (!ret) {
+ /* DOM was not readable after all */
+ memset(data + i, 0, ee->len - i);
return 0;
+ }
if (ret < 0) {
en_err(priv,
"mlx4_get_module_info i(%d) offset(%d)
bytes_to_read(%d) - FAILED (0x%x)\n",
i, offset, ee->len - i, ret);
- return 0;
+ return ret;
}
i += ret;
signature.asc
Description: PGP signature
Re: tc qdisc kernel crash
Control: tag -1 confirmed upstream Control: found -1 4.20-1~exp1 Adrian (cc'd) reported (https://bugs.debian.org/921542) that a script using tc could trigger a kernel crash. I've simplified the script he provided down to: --- BEGIN --- #!/bin/sh -ex modprobe ifb while true; do tc qdisc add dev ifb0 root handle 2:0 prio bands 5 tc qdisc add dev ifb0 parent 2:5 sfq tc filter add dev ifb0 parent 2:0 protocol ip prio 5 handle 0 tcindex mask 0 classid 2:5 pass_on tc qdisc del dev ifb0 root || true done --- END --- The crash is still reproducible in 4.20 and 5.0-rc5. KASan shows a use-after-free: + modprobe ifb + true + tc qdisc add dev ifb0 root handle 2:0 prio bands 5 + tc qdisc add dev ifb0 parent 2:5 sfq + tc filter add dev ifb0 parent 2:0 protocol ip prio 5 handle 0 tcindex mask 0 classid 2:5 pass_on + tc qdisc del dev ifb0 root + true + tc qdisc add dev ifb0 root handle 2:0 prio bands 5 [ 63.926983] == [ 63.929429] BUG: KASAN: use-after-free in worker_thread+0x327/0x5b0 [ 63.931489] Read of size 8 at addr 88804fd22130 by task kworker/u8:1/32 [ 63.933766] [ 63.934397] CPU: 0 PID: 32 Comm: kworker/u8:1 Not tainted 5.0.0-rc5 #3 [ 63.936629] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-1 04/01/2014 [ 63.939537] Workqueue:(null) (events_unbound) [ 63.942039] Call Trace: [ 63.943187] dump_stack+0x71/0xa0 [ 63.944386] ? worker_thread+0x327/0x5b0 [ 63.945881] print_address_description+0x65/0x22e [ 63.947980] ? worker_thread+0x327/0x5b0 [ 63.949588] ? worker_thread+0x327/0x5b0 [ 63.951254] kasan_report.cold.3+0x1a/0x40 [ 63.953036] ? worker_thread+0x327/0x5b0 [ 63.954692] worker_thread+0x327/0x5b0 [ 63.956236] ? flush_rcu_work+0x40/0x40 [ 63.957722] kthread+0x1ae/0x1d0 [ 63.959067] ? __kthread_parkme+0x90/0x90 [ 63.960451] ret_from_fork+0x35/0x40 [ 63.962020] [ 63.962817] Allocated by task 757: [ 63.964465] __kasan_kmalloc.constprop.13+0xc1/0xd0 [ 63.966670] tcindex_alloc_perfect_hash+0x37/0x150 [cls_tcindex] [ 63.969287] tcindex_set_parms+0xb38/0xd30 [cls_tcindex] [ 63.972539] tcindex_change+0x13d/0x1c2 [cls_tcindex] [ 63.974796] tc_new_tfilter+0x7ec/0xaf0 [ 63.976546] rtnetlink_rcv_msg+0x35c/0x490 [ 63.978302] netlink_rcv_skb+0xc6/0x1f0 [ 63.980050] netlink_unicast+0x309/0x3d0 [ 63.981990] netlink_sendmsg+0x37d/0x5e0 [ 63.983849] sock_sendmsg+0x6d/0x80 [ 63.985538] ___sys_sendmsg+0x46a/0x4e0 [ 63.987328] __sys_sendmsg+0xd3/0x160 [ 63.988974] do_syscall_64+0x73/0x140 [ 63.990616] entry_SYSCALL_64_after_hwframe+0x44/0xa9 [ 63.992538] [ 63.993430] Freed by task 9: [ 63.994660] __kasan_slab_free+0x125/0x170 [ 63.996239] kfree+0x90/0x1d0 [ 63.997496] __tcindex_destroy+0x1f/0x40 [cls_tcindex] [ 63.999316] rcu_process_callbacks+0x3cb/0x650 [ 64.000889] __do_softirq+0x115/0x3b4 [ 64.003254] [ 64.004138] The buggy address belongs to the object at 88804fd22100 [ 64.004138] which belongs to the cache kmalloc-8k of size 8192 [ 64.009001] The buggy address is located 48 bytes inside of [ 64.009001] 8192-byte region [88804fd22100, 88804fd24100) [ 64.013752] The buggy address belongs to the page: [ 64.015906] page:ea00013f4800 count:1 mapcount:0 mapping:888051002700 index:0x0 compound_mapcount: 0 [ 64.020237] flags: 0xc10200(slab|head) [ 64.022176] raw: 00c10200 dead0100 dead0200 888051002700 [ 64.025247] raw: 80030003 0001 [ 64.028847] page dumped because: kasan: bad access detected [ 64.031367] [ 64.033285] Memory state around the buggy address: [ 64.035276] 88804fd22000: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 64.037741] 88804fd22080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc [ 64.040138] >88804fd22100: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [ 64.042717] ^ [ 64.044794] 88804fd22180: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [ 64.047431] 88804fd22200: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb [ 64.049993] == Ben. -- Ben Hutchings The world is coming to an end. Please log off. signature.asc Description: This is a digitally signed message part
GSO where gso_size is too big for hardware
Last year you applied these fixes for a potential denial-of-service in the bnx2x driver: commit 2b16f048729bf35e6c28a40cbfad07239f9dcd90 Author: Daniel Axtens Date: Wed Jan 31 14:15:33 2018 +1100 net: create skb_gso_validate_mac_len() commit 8914a595110a6eca69a5e275b323f5d09e18f4f9 Author: Daniel Axtens Date: Wed Jan 31 14:15:34 2018 +1100 bnx2x: disable GSO where gso_size is too big for hardware However I don't understand why the check is done only in the bnx2x driver. Shouldn't the networking core ensure that gso_size + L3/L4 headers is <= the device MTU? If not, is every driver that does TSO expected to check this? Also, should these fixes go to stable? I'm not sure whether you're still handling stable patches for any of the unfixed versions (< 4.16) now. Ben. -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom
Re: [PATCH] ethtool: change to new sane powerpc64 kernel headers
On Fri, 2018-12-14 at 17:19 -0800, Maciej Żenczykowski wrote: > From: Maciej Żenczykowski > > This fixes: > In file included from ethtool-copy.h:22:0, >from internal.h:32, >from ethtool.c:29: > .../include/linux/types.h:32:25: error: conflicting types for '__be64' >typedef __u64 __bitwise __be64; >^ > In file included from ethtool.c:29:0: > internal.h:23:28: note: previous declaration of '__be64' was here >typedef unsigned long long __be64; > ^ > ethtool.c: In function 'do_gstats': > ethtool.c:3166:4: error: format '%llu' expects argument of type 'long long > unsigned int', but argument 5 has type '__u64' [-Werror=format=] > stats->data[i]); > ^ > ethtool.c: In function 'print_indir_table': > ethtool.c:3293:9: error: format '%llu' expects argument of type 'long long > unsigned int', but argument 3 has type '__u64' [-Werror=format=] >ctx->devname, ring_count->data); >^ > > $ gcc -dM -E - <<< "" | egrep -i 'power|ppc|arm|aarch|x86|86|amd' > > .#define __x86_64 1 > .#define __amd64 1 > .#define __x86_64__ 1 > .#define __amd64__ 1 > > .#define _ARCH_PPCGR 1 > .#define __PPC64__ 1 > .#define _ARCH_PPC 1 > .#define __powerpc64__ 1 > .#define __PPC__ 1 > .#define __powerpc__ 1 > .#define _ARCH_PPC64 1 > > .#define __AARCH64_CMODEL_SMALL__ 1 > .#define __aarch64__ 1 > .#define __AARCH64EL__ 1 > .#define __ARM_NEON 1 > > Signed-off-by: Maciej Żenczykowski > --- > ethtool-copy.h | 6 ++ > 1 file changed, 6 insertions(+) > > diff --git a/ethtool-copy.h b/ethtool-copy.h > index 6bfbb85f9402..7772a4970987 100644 > --- a/ethtool-copy.h > +++ b/ethtool-copy.h > @@ -14,6 +14,12 @@ > #ifndef _LINUX_ETHTOOL_H > #define _LINUX_ETHTOOL_H > > +#ifdef __powerpc64__ > +/* Powerpc needs __SANE_USERSPACE_TYPES__ before to select > + * 'int-ll64.h' and avoid compile warnings when printing __u64 with %llu. > + */ > +#define __SANE_USERSPACE_TYPES__ > +#endif > #include > #include > #include -- Ben Hutchings If you seem to know what you are doing, you'll be given more to do. signature.asc Description: This is a digitally signed message part
Re: [PATCH] ethtool: zero initialize coalesce struct
I handed over ethtool to John Linville some time ago.
Ben.
On Fri, 2018-12-14 at 17:19 -0800, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski
>
> prior to fetching it from kernel.
>
> Otherwise we run the risk of very tail portion of it (dmac field)
> being left entirely uninitialized, and likely containing some sort
> of stale data.
>
> It seems to likely be some sort of time (a second's counter).
>
> Tested:
> 'ethtool -c eth1' with old kernel now reports 'dmac: 0' where
> previously it reported some sort of second counter.
>
> Signed-off-by: Maciej Żenczykowski
> ---
> ethtool.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ethtool.c b/ethtool.c
> index 2f7e96bb58db..465eeecb9318 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -2076,7 +2076,7 @@ static int do_gchannels(struct cmd_context *ctx)
>
> static int do_gcoalesce(struct cmd_context *ctx)
> {
> - struct ethtool_coalesce ecoal;
> + struct ethtool_coalesce ecoal = {};
> int err;
>
> if (ctx->argc != 0)
--
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/3] bpf/verifier: Log instruction patching when verbose logging is enabled
On Fri, 2018-11-23 at 21:10 +0100, Daniel Borkmann wrote: > On 11/23/2018 07:34 PM, Ben Hutchings wrote: > > User-space does not have access to the patched eBPF code, but we > > need to be able to test that patches are being applied. Therefore > > log distinct messages for each case that requires patching. > > Thanks for the patches, Ben! Above is actually not the case, e.g. privileged > admin can use something like 'bpftool prog dump xlated id ' and then the > BPF insns are dumped to user space for the program /after/ the verification, > so the rewrites can then be seen. Oh that's good. > test_verifier temporarily drops caps to > load and run the unprivileged cases, but we can extend the test suite to > retrieve and check the final insns after that happened. I think this would be > a nice extension to the test suite for cases like these and would also provide > better insight than verbose() statement saying that something has been > patched (but not the actual result of it). Agreed; I'll look into doing this instead. Ben. -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom
[PATCH 2/3] selftests/bpf: Add the ability to test for a log message on success
This is needed to test that code is being patched when it should be.
Signed-off-by: Ben Hutchings
---
tools/testing/selftests/bpf/test_verifier.c | 11 ++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/test_verifier.c
b/tools/testing/selftests/bpf/test_verifier.c
index 0f3f97a401c9..e71b7f2e5f17 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -76,6 +76,8 @@ struct bpf_test {
int fixup_percpu_cgroup_storage[MAX_FIXUPS];
const char *errstr;
const char *errstr_unpriv;
+ const char *infostr;
+ const char *infostr_unpriv;
uint32_t retval, retval_unpriv;
enum {
UNDEF,
@@ -14232,7 +14234,7 @@ static void do_test_single(struct bpf_test *test, bool
unpriv,
int prog_len, prog_type = test->prog_type;
struct bpf_insn *prog = test->insns;
int map_fds[MAX_NR_MAPS];
- const char *expected_err;
+ const char *expected_err, *expected_info;
uint32_t expected_val;
uint32_t retval;
int i, err;
@@ -14253,6 +14255,8 @@ static void do_test_single(struct bpf_test *test, bool
unpriv,
test->result_unpriv : test->result;
expected_err = unpriv && test->errstr_unpriv ?
test->errstr_unpriv : test->errstr;
+ expected_info = unpriv && test->infostr_unpriv ?
+ test->infostr_unpriv : test->infostr;
expected_val = unpriv && test->retval_unpriv ?
test->retval_unpriv : test->retval;
@@ -14272,6 +14276,11 @@ static void do_test_single(struct bpf_test *test, bool
unpriv,
strerror(errno));
goto fail_log;
}
+ if (expected_info && !strstr(bpf_vlog, expected_info)) {
+ printf("FAIL\nMissing expected info message!\n\tEXP:
%s\n\tRES: %s\n",
+ expected_info, bpf_vlog);
+ goto fail_log;
+ }
} else {
if (fd_prog >= 0) {
printf("FAIL\nUnexpected success to load!\n");
--
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom
[PATCH 3/3] selftests/bpf: Add test case for defence against SSB exploitation
Test that the defence added by commit af86ca4e3088 "bpf: Prevent
memory disambiguation attack" is actually being applied.
Signed-off-by: Ben Hutchings
---
tools/testing/selftests/bpf/test_verifier.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_verifier.c
b/tools/testing/selftests/bpf/test_verifier.c
index e71b7f2e5f17..ca21a63541b0 100644
--- a/tools/testing/selftests/bpf/test_verifier.c
+++ b/tools/testing/selftests/bpf/test_verifier.c
@@ -13927,6 +13927,21 @@ static struct bpf_test tests[] = {
.result = ACCEPT,
},
{
+ "reference tracking: defend against SSB exploitation",
+ .insns = {
+ BPF_MOV32_IMM(BPF_REG_2, 1),
+ /* stack[-1] = (integer) 1 */
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_2, -8),
+ /* stack[-1] = (pointer) context */
+ BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_1, -8),
+ BPF_MOV32_IMM(BPF_REG_0, 0),
+ BPF_EXIT_INSN(),
+ },
+ .infostr_unpriv = "patching in sanitization against SSB at 2",
+ .result_unpriv = ACCEPT,
+ .result = ACCEPT,
+ },
+ {
"calls: ctx read at start of subprog",
.insns = {
BPF_MOV64_REG(BPF_REG_6, BPF_REG_1),
--
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom
[PATCH 1/3] bpf/verifier: Log instruction patching when verbose logging is enabled
User-space does not have access to the patched eBPF code, but we
need to be able to test that patches are being applied. Therefore
log distinct messages for each case that requires patching.
Signed-off-by: Ben Hutchings
---
kernel/bpf/verifier.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 4ce049cd30a3..ea4bc796e545 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5844,6 +5844,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env
*env)
verbose(env, "bpf verifier is misconfigured\n");
return -EINVAL;
} else if (cnt) {
+ verbose(env, "patching in prologue\n");
new_prog = bpf_patch_insn_data(env, 0, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -5892,6 +5893,9 @@ static int convert_ctx_accesses(struct bpf_verifier_env
*env)
};
cnt = ARRAY_SIZE(patch);
+ verbose(env,
+ "patching in sanitization against SSB at %d\n",
+ i + delta);
new_prog = bpf_patch_insn_data(env, i + delta, patch,
cnt);
if (!new_prog)
return -ENOMEM;
@@ -5973,6 +5977,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env
*env)
}
}
+ verbose(env, "patching explicit ctx access at %d\n", i + delta);
new_prog = bpf_patch_insn_data(env, i + delta, insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -6225,6 +6230,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
cnt = ARRAY_SIZE(mask_and_mod) - (is64 ? 1 : 0);
}
+ verbose(env, "patching in divide-by-zero check at %d\n",
+ i + delta);
new_prog = bpf_patch_insn_data(env, i + delta,
patchlet, cnt);
if (!new_prog)
return -ENOMEM;
@@ -6244,6 +6251,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
return -EINVAL;
}
+ verbose(env, "patching implicit ctx access at %d\n",
+ i + delta);
new_prog = bpf_patch_insn_data(env, i + delta,
insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -6307,6 +6316,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
map)->index_mask);
insn_buf[2] = *insn;
cnt = 3;
+ verbose(env, "patching in tail-call bounds check at %d",
+ i + delta);
new_prog = bpf_patch_insn_data(env, i + delta,
insn_buf, cnt);
if (!new_prog)
return -ENOMEM;
@@ -6342,6 +6353,8 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
return -EINVAL;
}
+ verbose(env, "patching in map lookup at %d",
+ i + delta);
new_prog = bpf_patch_insn_data(env, i + delta,
insn_buf, cnt);
if (!new_prog)
--
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom
[PATCH 0/3] bpf: Test defence against SSB exploitation
This series adds log messages for all patching done by the verifier, and a test case to verify that the patch to defend against SSB exploitation is applied where needed. Ben. Ben Hutchings (3): bpf/verifier: Log instruction patching when verbose logging is enabled selftests/bpf: Add the ability to test for a log message on success selftests/bpf: Add test case for defence against SSB exploitation kernel/bpf/verifier.c | 13 + tools/testing/selftests/bpf/test_verifier.c | 26 +- 2 files changed, 38 insertions(+), 1 deletion(-) -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom
[RFC PATCH] skb: Define NET_IP_ALIGN based on CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS
NET_IP_ALIGN is supposed to be defined as 0 if DMA writes to an unaligned buffer would be more expensive than CPU access to unaligned header fields, and otherwise defined as 2. Currently only ppc64 and x86 configurations define it to be 0. However several other architectures (conditionally) define CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS, which seems to imply that NET_IP_ALIGN should be 0. Remove the overriding definitions for ppc64 and x86 and define NET_IP_ALIGN solely based on CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS. Signed-off-by: Ben Hutchings --- arch/powerpc/include/asm/processor.h | 11 --- arch/x86/include/asm/processor.h | 8 include/linux/skbuff.h | 7 +++ 3 files changed, 3 insertions(+), 23 deletions(-) diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h index 52fadded5c1e..65c8210d2787 100644 --- a/arch/powerpc/include/asm/processor.h +++ b/arch/powerpc/include/asm/processor.h @@ -525,17 +525,6 @@ extern void cvt_fd(float *from, double *to); extern void cvt_df(double *from, float *to); extern void _nmask_and_or_msr(unsigned long nmask, unsigned long or_val); -#ifdef CONFIG_PPC64 -/* - * We handle most unaligned accesses in hardware. On the other hand - * unaligned DMA can be very expensive on some ppc64 IO chips (it does - * powers of 2 writes until it reaches sufficient alignment). - * - * Based on this we disable the IP header alignment in network drivers. - */ -#define NET_IP_ALIGN 0 -#endif - #endif /* __KERNEL__ */ #endif /* __ASSEMBLY__ */ #endif /* _ASM_POWERPC_PROCESSOR_H */ diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h index d53c54b842da..0108efc9726e 100644 --- a/arch/x86/include/asm/processor.h +++ b/arch/x86/include/asm/processor.h @@ -33,14 +33,6 @@ struct vm86; #include #include -/* - * We handle most unaligned accesses in hardware. On the other hand - * unaligned DMA can be quite expensive on some Nehalem processors. - * - * Based on this we disable the IP header alignment in network drivers. - */ -#define NET_IP_ALIGN 0 - #define HBP_NUM 4 /* * Default implementation of macro that returns current diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h index 17a13e4785fc..42467be8021f 100644 --- a/include/linux/skbuff.h +++ b/include/linux/skbuff.h @@ -2435,11 +2435,10 @@ static inline int pskb_network_may_pull(struct sk_buff *skb, unsigned int len) * The downside to this alignment of the IP header is that the DMA is now * unaligned. On some architectures the cost of an unaligned DMA is high * and this cost outweighs the gains made by aligning the IP header. - * - * Since this trade off varies between architectures, we allow NET_IP_ALIGN - * to be overridden. */ -#ifndef NET_IP_ALIGN +#ifdef CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS +#define NET_IP_ALIGN 0 +#else #define NET_IP_ALIGN 2 #endif -- Ben Hutchings, Software Developer Codethink Ltd https://www.codethink.co.uk/ Dale House, 35 Dale Street Manchester, M1 2HF, United Kingdom
Re: GPL compliance issue with liquidio/lio_23xx_vsw.bin firmware
On Mon, 2018-08-27 at 17:04 -0700, Felix Manlunas wrote: > On Mon, Aug 27, 2018 at 05:01:10PM +0200, Florian Weimer wrote: > > liquidio/lio_23xx_vsw.bin contains a compiled MIPS Linux kernel: > > > > $ tail --bytes=+1313 liquidio/lio_23xx_vsw.bin > elf > > $ readelf -aW elf > > […] > > [ 6] __ksymtab PROGBITS80e495f8 64a5f8 00d130 > > 00 A 0 0 8 > > [ 7] __ksymtab_gpl PROGBITS80e56728 657728 008400 > > 00 A 0 0 8 > > [ 8] __ksymtab_strings PROGBITS80e5eb28 65fb28 018868 > > 00 A 0 0 1 > > […] > > Symbol table '.symtab' contains 1349 entries: > >Num:Value Size TypeBind Vis Ndx Name > > 0: 0 NOTYPE LOCAL DEFAULT UND > > 1: 0 FILELOCAL DEFAULT ABS > > arch/mips/kernel/head.o > > 2: 0 FILELOCAL DEFAULT ABS init/main.c > > 3: 0 FILELOCAL DEFAULT ABS > > include/linux/types.h > > […] > > > > Yet there is no corresponding source provided, and LICENCE.cavium lacks > > the required notices. > > > > Thanks, > > Florian > > Cavium apologizes for the oversight. Cavium has been advertising the > appropriate license terms including the existence of GPL in the firmware > in our outbox releases. We will update the license terms in LICENCE.cavium > in our upstream contribution in collaboration with our legal team. Everything added to linux-firmware.git needs to be safe for Linux distributions to redistribute. (There are some ancient firmware images with unclear licensing, but we don't want to add any more.) My understanding is that GPL v2 requires that commercial distributors either provide the complete and corresponding source alongside the binaries, or include a written offer to provide it later. They *cannot* simply point to your offer to do this (only non-commercial distributors can do that). So the source needs to be published too. Adding the complete Linux kernel source code to linux-firmware.git doesn't seem like a sensible step, so maybe this particular firmware needs to live elsewhere. Ben. -- Ben Hutchings For every complex problem there is a solution that is simple, neat, and wrong. signature.asc Description: This is a digitally signed message part
Re: [linux-stable-3.16.y] tun: allow positive return values on dev_get_valid_name() call
On Mon, 2018-05-14 at 15:47 +0200, Sedat Dilek wrote: > Hi Ben, > > some Debian/jessie systems were caught by the bug-report in [1]. > This issue was recently fixed in an updated Debian kernel for v3.16.y. > > Will you include the patch "tun: allow positive return values on > dev_get_valid_name() call" [2] in linux-stable-3.16.y upstream? Yes, I will include all the same regression fixes in the next 3.16- stable update. Ben. > This was a fix for "tun: call dev_get_valid_name() before > register_netdevice()" [3]. > Unfortunately, there is not a reference (usually "Fixes:" tag) for this in > [2]. > Not sure if this was documented in the meantime like [4] says. > > Thanks in advance. > > Regards, > - Sedat - > > [1] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=897427 > [2] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5c25f65fd1e42685f7ccd80e0621829c105785d9 > [3] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0ad646c81b2182f7fa67ec0c8c825e0ee165696d > [4] https://www.spinics.net/lists/netdev/msg462705.html -- Ben Hutchings For every action, there is an equal and opposite criticism. - Harrison signature.asc Description: This is a digitally signed message part
[PATCH 2/2] hns: Clean up string operations
The driver-internal string operations are only ever used for
statistics, so remove the stringset parameters and rename them
accordingly.
Signed-off-by: Ben Hutchings
---
drivers/net/ethernet/hisilicon/hns/hnae.h | 13
drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c | 37 +++---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_gmac.c | 16 +++---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 9 +++---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.h | 10 +++---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 28 ++--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h | 6 ++--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.c | 11 +++
drivers/net/ethernet/hisilicon/hns/hns_dsaf_ppe.h | 4 +--
drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.c | 20
drivers/net/ethernet/hisilicon/hns/hns_dsaf_rcb.h | 4 +--
.../net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c| 24 +-
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 9 +++---
13 files changed, 78 insertions(+), 113 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hnae.h
b/drivers/net/ethernet/hisilicon/hns/hnae.h
index 3e62692af011..480fa2b49be7 100644
--- a/drivers/net/ethernet/hisilicon/hns/hnae.h
+++ b/drivers/net/ethernet/hisilicon/hns/hnae.h
@@ -457,10 +457,10 @@ enum hnae_media_type {
* update Old network device statistics
* get_ethtool_stats()
* get ethtool network device statistics
- * get_strings()
- * get a set of strings that describe the requested objects
- * get_sset_count()
- * get number of strings that @get_strings will write
+ * get_stats_strings()
+ * get a set of strings that describe the statistics
+ * get_stats_count()
+ * get number of strings that @get_stats_strings will write
* update_led_status()
* update the led status
* set_led_id()
@@ -522,9 +522,8 @@ struct hnae_ae_ops {
void (*update_stats)(struct hnae_handle *handle,
struct net_device_stats *net_stats);
void (*get_stats)(struct hnae_handle *handle, u64 *data);
- void (*get_strings)(struct hnae_handle *handle,
- u32 stringset, u8 *data);
- int (*get_sset_count)(struct hnae_handle *handle, int stringset);
+ void (*get_stats_strings)(struct hnae_handle *handle, u8 *data);
+ int (*get_stats_count)(struct hnae_handle *handle);
void (*update_led_status)(struct hnae_handle *handle);
int (*set_led_id)(struct hnae_handle *handle,
enum hnae_led_state status);
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
index bd68379d2bea..638476aa6311 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ae_adapt.c
@@ -679,21 +679,20 @@ void hns_ae_get_stats(struct hnae_handle *handle, u64
*data)
for (idx = 0; idx < handle->q_num; idx++) {
hns_rcb_get_stats(handle->qs[idx], p);
- p += hns_rcb_get_ring_sset_count((int)ETH_SS_STATS);
+ p += hns_rcb_get_ring_stats_count();
}
hns_ppe_get_stats(ppe_cb, p);
- p += hns_ppe_get_sset_count((int)ETH_SS_STATS);
+ p += hns_ppe_get_stats_count();
hns_mac_get_stats(mac_cb, p);
- p += hns_mac_get_sset_count(mac_cb, (int)ETH_SS_STATS);
+ p += hns_mac_get_stats_count(mac_cb);
if (mac_cb->mac_type == HNAE_PORT_SERVICE)
hns_dsaf_get_stats(vf_cb->dsaf_dev, p, vf_cb->port_index);
}
-void hns_ae_get_strings(struct hnae_handle *handle,
- u32 stringset, u8 *data)
+void hns_ae_get_stats_strings(struct hnae_handle *handle, u8 *data)
{
int port;
int idx;
@@ -711,21 +710,21 @@ void hns_ae_get_strings(struct hnae_handle *handle,
ppe_cb = hns_get_ppe_cb(handle);
for (idx = 0; idx < handle->q_num; idx++) {
- hns_rcb_get_strings(stringset, p, idx);
- p += ETH_GSTRING_LEN * hns_rcb_get_ring_sset_count(stringset);
+ hns_rcb_get_stats_strings(p, idx);
+ p += ETH_GSTRING_LEN * hns_rcb_get_ring_stats_count();
}
- hns_ppe_get_strings(ppe_cb, stringset, p);
- p += ETH_GSTRING_LEN * hns_ppe_get_sset_count(stringset);
+ hns_ppe_get_stats_strings(ppe_cb, p);
+ p += ETH_GSTRING_LEN * hns_ppe_get_stats_count();
- hns_mac_get_strings(mac_cb, stringset, p);
- p += ETH_GSTRING_LEN * hns_mac_get_sset_count(mac_cb, stringset);
+ hns_mac_get_stats_strings(mac_cb, p);
+ p += ETH_GSTRING_LEN * hns_mac_get_stats_count(mac_cb);
if (mac_cb->mac_type == HNAE_PORT_SERVICE)
- hns_dsaf_get_strings(stringset, p, port, dsaf_dev);
+ hns_dsaf_get_stats_strings(p, port, dsaf_dev);
}
-int hns_ae_get_sset_count(struct hnae_handle *
[PATCH net 1/2] hns: Fix string set validation in ethtool string operations
The hns driver used to assume that it would only ever be asked
about ETH_SS_TEST or ETH_SS_STATS, and for any other stringset
it would claim a count of 0 but if asked for names would copy
over all the statistic names. This resulted in a potential
heap buffer overflow.
This was "fixed" some time ago by making it report the number of
statistics when asked about the ETH_SS_PRIV_FLAGS stringset. This
doesn't make any sense, and it left the bug still exploitable with
other stringset numbers.
As a minimal but complete fix, stop calling the driver-internal
string operations for any stringset other than ETH_SS_STATS.
Fixes: 511e6bc071db ("net: add Hisilicon Network Subsystem DSAF support")
Fixes: 412b65d15a7f ("net: hns: fix ethtool_get_strings overflow ...")
Signed-off-by: Ben Hutchings
---
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index 7ea7f8a4aa2a..b836742f7ab6 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -889,11 +889,6 @@ void hns_get_strings(struct net_device *netdev, u32
stringset, u8 *data)
struct hnae_handle *h = priv->ae_handle;
char *buff = (char *)data;
- if (!h->dev->ops->get_strings) {
- netdev_err(netdev, "h->dev->ops->get_strings is null!\n");
- return;
- }
-
if (stringset == ETH_SS_TEST) {
if (priv->ae_handle->phy_if != PHY_INTERFACE_MODE_XGMII) {
memcpy(buff, hns_nic_test_strs[MAC_INTERNALLOOP_MAC],
@@ -907,7 +902,7 @@ void hns_get_strings(struct net_device *netdev, u32
stringset, u8 *data)
memcpy(buff, hns_nic_test_strs[MAC_INTERNALLOOP_PHY],
ETH_GSTRING_LEN);
- } else {
+ } else if (stringset == ETH_SS_STATS && h->dev->ops->get_strings) {
snprintf(buff, ETH_GSTRING_LEN, "rx_packets");
buff = buff + ETH_GSTRING_LEN;
snprintf(buff, ETH_GSTRING_LEN, "tx_packets");
@@ -969,7 +964,7 @@ void hns_get_strings(struct net_device *netdev, u32
stringset, u8 *data)
/**
* nic_get_sset_count - get string set count witch returned by nic_get_strings.
* @dev: net device
- * @stringset: string set index, 0: self test string; 1: statistics string.
+ * @stringset: string set index
*
* Return string set count.
*/
@@ -979,10 +974,6 @@ int hns_get_sset_count(struct net_device *netdev, int
stringset)
struct hnae_handle *h = priv->ae_handle;
struct hnae_ae_ops *ops = h->dev->ops;
- if (!ops->get_sset_count) {
- netdev_err(netdev, "get_sset_count is null!\n");
- return -EOPNOTSUPP;
- }
if (stringset == ETH_SS_TEST) {
u32 cnt = (sizeof(hns_nic_test_strs) / ETH_GSTRING_LEN);
@@ -993,8 +984,10 @@ int hns_get_sset_count(struct net_device *netdev, int
stringset)
cnt--;
return cnt;
+ } else if (stringset == ETH_SS_STATS && ops->get_sset_count) {
+ return HNS_NET_STATS_CNT + ops->get_sset_count(h, stringset);
} else {
- return (HNS_NET_STATS_CNT + ops->get_sset_count(h, stringset));
+ return -EOPNOTSUPP;
}
}
signature.asc
Description: Digital signature
[stable] dccp: CVE-2017-8824: use-after-free in DCCP code
Please queue up this commit for stable: 69c64866ce07 dccp: CVE-2017-8824: use-after-free in DCCP code Ben. -- Ben Hutchings Software Developer, Codethink Ltd.
[PATCH net] ipv6: Fix getsockopt() for sockets with default IPV6_AUTOFLOWLABEL
Commit 513674b5a2c9 ("net: reevalulate autoflowlabel setting after
sysctl setting") removed the initialisation of
ipv6_pinfo::autoflowlabel and added a second flag to indicate
whether this field or the net namespace default should be used.
The getsockopt() handling for this case was not updated, so it
currently returns 0 for all sockets for which IPV6_AUTOFLOWLABEL is
not explicitly enabled. Fix it to return the effective value, whether
that has been set at the socket or net namespace level.
Fixes: 513674b5a2c9 ("net: reevalulate autoflowlabel setting after sysctl ...")
Signed-off-by: Ben Hutchings
---
This will need to go to stable, as the earlier commit did.
Ben.
include/net/ipv6.h | 1 +
net/ipv6/ip6_output.c| 2 +-
net/ipv6/ipv6_sockglue.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index f73797e2fa60..221238254eb7 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -331,6 +331,7 @@ int ipv6_flowlabel_opt_get(struct sock *sk, struct
in6_flowlabel_req *freq,
int flags);
int ip6_flowlabel_init(void);
void ip6_flowlabel_cleanup(void);
+bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np);
static inline void fl6_sock_release(struct ip6_flowlabel *fl)
{
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index 688ba5f7516b..08446e0ca411 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -166,7 +166,7 @@ int ip6_output(struct net *net, struct sock *sk, struct
sk_buff *skb)
!(IP6CB(skb)->flags & IP6SKB_REROUTED));
}
-static bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
+bool ip6_autoflowlabel(struct net *net, const struct ipv6_pinfo *np)
{
if (!np->autoflowlabel_set)
return ip6_default_np_autolabel(net);
diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 2d4680e0376f..e8ffb5b5d84e 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -1336,7 +1336,7 @@ static int do_ipv6_getsockopt(struct sock *sk, int level,
int optname,
break;
case IPV6_AUTOFLOWLABEL:
- val = np->autoflowlabel;
+ val = ip6_autoflowlabel(sock_net(sk), np);
break;
case IPV6_RECVFRAGSIZE:
--
2.15.0.rc0
Re: [PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path
On Wed, 2018-01-10 at 14:25 -0800, Cong Wang wrote: > On Tue, Jan 9, 2018 at 10:21 AM, Ben Hutchings > wrote: > > Commit 15e668070a64 reordered the initialisation in inet6_init() to > > fix a crash on an error path further down the call stack. It also > > reordered cleanup on the error path in inet6_init(), but the result > > is not the reverse of the initialisation order. This presumably > > can result in a resource leak or crash in some error > > cases. Reorder > > cleanup again to fix this. > > Can you be specific on what resource we leak here? If icmpv6_init() fails, after ip6_mr_init(), then ip6_mr_cleanup() is not called. Also, if ip6_mr_init() fails, we don't unregister inet6_net_ops. I think that will result in a crash - immediately if ipv6 is a module, otherwise when the next net namespace is created. > Also, it looks like you not just revert the order changed in commit > 15e668070a64, but also you move icmpv6_cleanup() even earlier. So should I add another Fixes: there? Ben. -- Ben Hutchings Software Developer, Codethink Ltd.
[PATCH net] ipv6: Fix cleanup ordering on inet6_init() error path
Commit 15e668070a64 reordered the initialisation in inet6_init() to
fix a crash on an error path further down the call stack. It also
reordered cleanup on the error path in inet6_init(), but the result
is not the reverse of the initialisation order. This presumably
can result in a resource leak or crash in some error cases. Reorder
cleanup again to fix this.
Fixes: 15e668070a64 ("ipv6: reorder icmpv6_init() and ip6_mr_init()")
Signed-off-by: Ben Hutchings
---
This fix is untested and based only on my review of the earlier commit.
Ben.
net/ipv6/af_inet6.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index c9441ca45399..fbaa70d95d7f 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -1074,11 +1074,11 @@ static int __init inet6_init(void)
igmp_fail:
ndisc_cleanup();
ndisc_fail:
- ip6_mr_cleanup();
+ icmpv6_cleanup();
icmp_fail:
- unregister_pernet_subsys(&inet6_net_ops);
+ ip6_mr_cleanup();
ipmr_fail:
- icmpv6_cleanup();
+ unregister_pernet_subsys(&inet6_net_ops);
register_pernet_fail:
sock_unregister(PF_INET6);
rtnl_unregister_all(PF_INET6);
--
2.15.0.rc0
[PATCH 4.9] bpf/verifier: Fix states_equal() comparison of pointer and UNKNOWN
An UNKNOWN_VALUE is not supposed to be derived from a pointer, unless pointer leaks are allowed. Therefore, states_equal() must not treat a state with a pointer in a register as "equal" to a state with an UNKNOWN_VALUE in that register. This was fixed differently upstream, but the code around here was largely rewritten in 4.14 by commit f1174f77b50c "bpf/verifier: rework value tracking". The bug can be detected by the bpf/verifier sub-test "pointer/scalar confusion in state equality check (way 1)". Signed-off-by: Ben Hutchings Cc: Edward Cree Cc: Jann Horn Cc: Alexei Starovoitov --- --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2722,11 +2722,12 @@ static bool states_equal(struct bpf_veri /* If we didn't map access then again we don't care about the * mismatched range values and it's ok if our old type was -* UNKNOWN and we didn't go to a NOT_INIT'ed reg. +* UNKNOWN and we didn't go to a NOT_INIT'ed or pointer reg. */ if (rold->type == NOT_INIT || (!varlen_map_access && rold->type == UNKNOWN_VALUE && -rcur->type != NOT_INIT)) +rcur->type != NOT_INIT && +!__is_pointer_value(env->allow_ptr_leaks, rcur))) continue; /* Don't care about the reg->id in this case. */ signature.asc Description: Digital signature
Re: pull request: Cavium Octeon III firmware
On Tue, 2017-11-28 at 11:09 -0600, Steven J. Hill wrote: > On 11/22/2017 07:40 PM, Ben Hutchings wrote: > > On Tue, 2017-10-31 at 17:05 -0500, Steven J. Hill wrote: > > > Hello. > > > > > > Would like to add firmware for our Octeon III PKI driver. Thanks. > > > > Where is this driver? I don't see any reference to the file in linux- > > next. > > > > [...] > > > cavium/pki-cluster.bin | Bin 0 -> 7488 bytes > > > 1 file changed, 0 insertions(+), 0 deletions(-) > > > create mode 100644 cavium/pki-cluster.bin > > > > When adding a file you also need to update WHENCE to include its > > copyright details. > > > > The WHENCE file was updated in the patch. I will assume you would like > to see two separate patches. One for the binary and one for the WHENCE. I would have preferred a single patch that did both. The diffstat in your original pull request said that WHENCE wasn't updated. Maybe you corrected that after sending the pull request. Ben. > I have done so and updated the git repo for you to pull from. > > > git://git.linux-mips.org/pub/scm/sjhill/linux-firmware.git -- Ben Hutchings Anthony's Law of Force: Don't force it, get a larger hammer. signature.asc Description: This is a digitally signed message part
Re: [PATCH linux-firmware 0/2] Mellanox: Add new mlxsw_spectrum firmware 13.1530.152
On Thu, 2017-11-09 at 09:15 +0200, Shalom Toledo wrote: > This set adds a new firmware version 13.1530.152 as well as information about > the previous firmware version of the mlxsw_spectrum driver > > Shalom Toledo (2): > WHENCE: Add missing entry for mlxsw_spectrum firmware > Mellanox: Add new mlxsw_spectrum firmware 13.1530.152 > > WHENCE | 38 > +++ > mellanox/mlxsw_spectrum-13.1530.152.mfa2 | Bin 0 -> 924020 bytes > 2 files changed, 38 insertions(+) > create mode 100644 mellanox/mlxsw_spectrum-13.1530.152.mfa2 Applied both of these, thanks. Ben. -- Ben Hutchings When in doubt, use brute force. - Ken Thompson signature.asc Description: This is a digitally signed message part
Re: pull-request Cavium LiquidIO firmware v1.7.0
On Mon, 2017-11-13 at 13:07 -0800, Felix Manlunas wrote: > The following changes since commit bf04291309d3169c0ad3b8db52564235bbd08e30: > > WHENCE: Add new qed firmware (2017-10-09 18:03:26 +0100) > > are available in the git repository at: > > https://github.com/felix-cavium/linux-firmware.git for-upstreaming-v1.7.0 > > for you to fetch changes up to 6c161c56d5bbf233f5931820193802a5bcb10356: > > linux-firmware: liquidio: update firmware to v1.7.0 (2017-11-07 17:11:40 > -0800) > > Signed-off-by: Felix Manlunas > Signed-off-by: Derek Chickles > > Felix Manlunas (1): > linux-firmware: liquidio: update firmware to v1.7.0 > > WHENCE | 8 > liquidio/lio_210nv_nic.bin | Bin 1261080 -> 1265368 bytes > liquidio/lio_210sv_nic.bin | Bin 1159096 -> 1163128 bytes > liquidio/lio_23xx_nic.bin | Bin 1266528 -> 1271456 bytes > liquidio/lio_410nv_nic.bin | Bin 1261080 -> 1265368 bytes > 5 files changed, 4 insertions(+), 4 deletions(-) Pulled, thanks. Ben. -- Ben Hutchings When in doubt, use brute force. - Ken Thompson signature.asc Description: This is a digitally signed message part
Re: pull request: Cavium Octeon III firmware
On Tue, 2017-10-31 at 17:05 -0500, Steven J. Hill wrote: > Hello. > > Would like to add firmware for our Octeon III PKI driver. Thanks. Where is this driver? I don't see any reference to the file in linux- next. [...] > cavium/pki-cluster.bin | Bin 0 -> 7488 bytes > 1 file changed, 0 insertions(+), 0 deletions(-) > create mode 100644 cavium/pki-cluster.bin When adding a file you also need to update WHENCE to include its copyright details. Ben. -- Ben Hutchings When in doubt, use brute force. - Ken Thompson signature.asc Description: This is a digitally signed message part
Re: [PATCH 1/1] qed: Add firmware 8.33.1.0
On Wed, 2017-10-11 at 00:57 -0700, Rahul Verma wrote: > The new qed firmware contains fixes to firmware and added > support for new features, > -Add UFP support and drop action support. > -DCQCN support for unlimited number of QP > -Add IP type to GFT filter profile. > -Added new TCP function counters. > -Support flow ID in aRFS flow. > > Signed-off-by: Rahul Verma > --- > WHENCE | 1 + > qed/qed_init_values_zipped-8.33.1.0.bin | Bin 0 -> 838612 bytes > 2 files changed, 1 insertion(+) > create mode 100755 qed/qed_init_values_zipped-8.33.1.0.bin [...] Applied; sorry for the delay. Ben. -- Ben Hutchings When in doubt, use brute force. - Ken Thompson signature.asc Description: This is a digitally signed message part
Re: scheduling while atomic from vmci_transport_recv_stream_cb in 3.16 kernels
On Wed, 2017-09-13 at 17:19 +0200, Michal Hocko wrote:
> On Wed 13-09-17 15:07:26, Jorgen S. Hansen wrote:
[...]
> > The patch below has been used to fix the above issue by other distros
> > - among them Redhat for the 3.10 kernel, so it should work for 3.16 as
> > well.
>
> Thanks for the confirmation. I do not see 4ef7ea9195ea ("VSOCK: sock_put
> wasn't safe to call in interrupt context") in 3.10 stable branch
> though.
>
> > In addition to the patch above, there are two other patches that
> > need to be applied on top for the fix to be correct:
> >
> > 8566b86ab9f0f45bc6f7dd422b21de9d0cf5415a "VSOCK: Fix lockdep issue."
> >
> > and
> >
> > 8ab18d71de8b07d2c4d6f984b718418c09ea45c5 "VSOCK: Detach QP check should
> > filter out non matching QPs."
>
> Good to know. I will send all three patches cherry-picked on top of the
> current 3.16 stable branch. Could you have a look please?
I've now queued these all up.
Ben.
--
Ben Hutchings
If you seem to know what you are doing, you'll be given more to do.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 4.4 27/56] cdc_ncm: Set NTB format again after altsetting switch for Huawei devices
On Tue, 2017-07-11 at 17:21 +0200, Enrico Mioso wrote:
> From: Enrico Mioso
>
> commit 2b02c20ce0c28974b44e69a2e2f5ddc6a470ad6f upstream.
[...]
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -724,8 +724,10 @@ int cdc_ncm_bind_common(struct usbnet *d
> u8 *buf;
> int len;
> int temp;
> + int err;
> u8 iface_no;
> struct usb_cdc_parsed_header hdr;
> + u16 curr_ntb_format;
[...]
> + err = usbnet_read_cmd(dev, USB_CDC_GET_NTB_FORMAT,
> + USB_TYPE_CLASS | USB_DIR_IN |
> USB_RECIP_INTERFACE,
> + 0, iface_no, &curr_ntb_format, 2);
> + if (err < 0) {
> + goto error2;
> + }
> +
> + if (curr_ntb_format == USB_CDC_NCM_NTB32_FORMAT) {
[...]
usbnet_read_cmd() doesn't do any byte-swapping, so it looks like
curr_ntb_format will have little-endian byte order (__le16 not u16).
The comparison will then need to be done using
le16_to_cpu(curr_ntb_format).
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
Re: [PATCH] net: recvmsg: Unconditionally zero struct sockaddr_storage
On Tue, 2017-10-31 at 09:14 -0700, Kees Cook wrote: > Some protocols do not correctly wipe the contents of the on-stack > struct sockaddr_storage sent down into recvmsg() (e.g. SCTP), and leak > kernel stack contents to userspace. This wipes it unconditionally before > per-protocol handlers run. > > Note that leaks like this are mitigated by building with > CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL=y > > Reported-by: Alexander Potapenko > Cc: "David S. Miller" > Cc: netdev@vger.kernel.org > Signed-off-by: Kees Cook > --- > net/socket.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/net/socket.c b/net/socket.c > index c729625eb5d3..34183f4fbdf8 100644 > --- a/net/socket.c > +++ b/net/socket.c > @@ -2188,6 +2188,7 @@ static int ___sys_recvmsg(struct socket *sock, struct > user_msghdr __user *msg, > struct sockaddr __user *uaddr; > int __user *uaddr_len = COMPAT_NAMELEN(msg); > > + memset(&addr, 0, sizeof(addr)); That's a fairly large structure (128 bytes), most of which won't normally be used. We already initialise msg_namelen to 0 before calling the per-protocol handler, which means by default nothing leaks. Only cases where msg_namelen is set but msg_name[] is not initialised up to that length are a problem. I would have thought they were not too hard to find and fix. Ben. > msg_sys->msg_name = &addr; > > if (MSG_CMSG_COMPAT & flags) > -- > 2.7.4 > > -- Ben Hutchings It is a miracle that curiosity survives formal education. - Albert Einstein signature.asc Description: This is a digitally signed message part
Re: [PATCH linux-firmware 1/1] qed: Add firmware 8.30.16.0
On Wed, 2017-09-13 at 03:46 -0700, Rahul Verma wrote: > The new qed firmware contains fixes to firmware and added > support for new features, > -Add UFP support. > -DCQCN support for unlimited number of QP > -Add IP type to GFT filter profile. > -Added new TCP function counters. > -Support flow ID in aRFS flow. > > Signed-off-by: Rahul Verma > --- > qed/qed_init_values_zipped-8.30.16.0.bin | Bin 0 -> 837008 bytes > 1 file changed, 0 insertions(+), 0 deletions(-) > create mode 100755 qed/qed_init_values_zipped-8.30.16.0.bin [...] The new file needs an entry in WHENCE. Ben. -- Ben Hutchings Humour is the best antidote to reality. signature.asc Description: This is a digitally signed message part
Re: pull request: linux-firmware: update cxgb4 firmware
On Wed, 2017-09-27 at 20:54 +0530, Ganesh Goudar wrote: > Hi, > > Kindly pull the new firmware from the following URL. > git://git.chelsio.net/pub/git/linux-firmware.git for-upstream Pulled, thanks. Ben. > Thanks > Ganesh > > The following changes since commit 59bf7e2cad88269ea704eb270d7ac6e69e8a544c: > > cxgb4: update firmware to revision 1.16.63.0 (2017-09-27 08:14:29 -0700) > > are available in the git repository at: > > git://git.chelsio.net/pub/git/linux-firmware.git for-upstream > > for you to fetch changes up to 59bf7e2cad88269ea704eb270d7ac6e69e8a544c: > > cxgb4: update firmware to revision 1.16.63.0 (2017-09-27 08:14:29 -0700) > > -------- -- Ben Hutchings Humour is the best antidote to reality. signature.asc Description: This is a digitally signed message part
Re: scheduling while atomic from vmci_transport_recv_stream_cb in 3.16 kernels
On Thu, 2017-09-14 at 10:59 +0200, Michal Hocko wrote: > On Wed 13-09-17 18:58:13, Jorgen S. Hansen wrote: > [...] > > The patch series look good to me. > > Thanks for double checking. Ben, could you merge this to 3.16 stable > branch, please? I have a long list of requests to work through, but I will get to this eventually. Ben. -- Ben Hutchings Kids! Bringing about Armageddon can be dangerous. Do not attempt it in your own home. - Terry Pratchett and Neil Gaiman, `Good Omens' signature.asc Description: This is a digitally signed message part
Re: WARNING in dev_watchdog
On Mon, 2017-06-12 at 16:13 +0800, Dison River wrote: > Sorry,this WARNING is not reproducible.And I don't have PoC for this > bug. I found and reported a similar bug in e1000 and e1000e some years ago, which was fixed in the latter here: https://git.kernel.org/linus/d821a4c4d11ad160925dab2bb009b8444beff484 I think e1000 may still be unfixed, so this might be the same bug. Ben. -- Ben Hutchings Unix is many things to many people, but it's never been everything to anybody. signature.asc Description: This is a digitally signed message part
Leak in ipv6_gso_segment()?
If I'm not mistaken, ipv6_gso_segment() now leaks segs if ip6_find_1stfragopt() fails. I'm not sure whether the fix would be as simple as adding a kfree_skb(segs) or whether more complex cleanup is needed. Ben. -- Ben Hutchings Lowery's Law: If it jams, force it. If it breaks, it needed replacing anyway. signature.asc Description: This is a digitally signed message part
[PATCH net] ipv6: xfrm: Handle errors reported by xfrm6_find_1stfragopt()
xfrm6_find_1stfragopt() may now return an error code and we must
not treat it as a length.
Fixes: 2423496af35d ("ipv6: Prevent overrun when parsing v6 header options")
Signed-off-by: Ben Hutchings
---
Commits 2423496af35d "ipv6: Prevent overrun when parsing v6 header
options" and 7dd7eb9513bd "ipv6: Check ip6_find_1stfragopt() return
value properly." changed ip6_find_1stfragopt() to return negative
error codes and changed some of its callers to handle this.
However, there is also xfrm6_find_1stfragopt() which is a wrapper for
ip6_find_1stfragopt() and is called indirectly by xfrm6_ro_output()
and xfrm6_transport_output(). Neither of them check for errors.
This is totally untested. I think xfrm_type::hdr_offset deserves a
comment about its semantics, but I don't understand it well enogugh to
write that.
Ben.
net/ipv6/xfrm6_mode_ro.c| 2 ++
net/ipv6/xfrm6_mode_transport.c | 2 ++
2 files changed, 4 insertions(+)
diff --git a/net/ipv6/xfrm6_mode_ro.c b/net/ipv6/xfrm6_mode_ro.c
index 0e015906f9ca..07d36573f50b 100644
--- a/net/ipv6/xfrm6_mode_ro.c
+++ b/net/ipv6/xfrm6_mode_ro.c
@@ -47,6 +47,8 @@ static int xfrm6_ro_output(struct xfrm_state *x, struct
sk_buff *skb)
iph = ipv6_hdr(skb);
hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
+ if (hdr_len < 0)
+ return hdr_len;
skb_set_mac_header(skb, (prevhdr - x->props.header_len) - skb->data);
skb_set_network_header(skb, -x->props.header_len);
skb->transport_header = skb->network_header + hdr_len;
diff --git a/net/ipv6/xfrm6_mode_transport.c b/net/ipv6/xfrm6_mode_transport.c
index 7a92c0f31912..9ad07a91708e 100644
--- a/net/ipv6/xfrm6_mode_transport.c
+++ b/net/ipv6/xfrm6_mode_transport.c
@@ -30,6 +30,8 @@ static int xfrm6_transport_output(struct xfrm_state *x,
struct sk_buff *skb)
skb_set_inner_transport_header(skb, skb_transport_offset(skb));
hdr_len = x->type->hdr_offset(x, skb, &prevhdr);
+ if (hdr_len < 0)
+ return hdr_len;
skb_set_mac_header(skb, (prevhdr - x->props.header_len) - skb->data);
skb_set_network_header(skb, -x->props.header_len);
skb->transport_header = skb->network_header + hdr_len;
signature.asc
Description: Digital signature
[stable] Networking security fixes
Please queue up these fixes for 4.10.y: 8605330aac5a tcp: fix SCM_TIMESTAMPING_OPT_STATS for normal skbs 4ef1b2869447 tcp: mark skbs with SCM_TIMESTAMPING_OPT_STATS If I understand correctly, you are no longer sending Greg fixes for 4.4.y or older stable branches, so I'll send stable requests for those older versions directly. Please can you also send the pending fixes to Greg soon, as the AF_PACKET issue is quite serious? Ben. -- Ben Hutchings Software Developer, Codethink Ltd.
Re: [PATCH net-stable] ipv4: keep skb->dst around in presence of IP options
On Mon, 2017-03-20 at 21:23 -0700, Eric Dumazet wrote:
> From: Eric Dumazet
>
> Upstream commit 34b2cef20f19c87999fff3da4071e66937db9644
> ("ipv4: keep skb->dst around in presence of IP options") incorrectly
> root caused commit d826eb14ecef ("ipv4: PKTINFO doesnt need dst
> reference") as bug origin.
>
> This patch should fix the issue for 3.2.xx stable kernels, since IPv4
> options seem to get more traction these days, after years of oblivion
> ;)
>
> Fixes: f84af32cbca70 ("net: ip_queue_rcv_skb() helper"))
> Signed-off-by: Eric Dumazet
> Reported-by: Anarcheuz Fritz
> ---
>
> This is a backport for 3.2 kernels.
Added to the queue, thanks.
Ben.
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index b3648bbef0da..a6e1eeb02267 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -1009,7 +1009,8 @@ e_inval:
> */
> int ip_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> {
> - if (!(inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO))
> + if (!(inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO) &&
> + !IPCB(skb)->opt.optlen)
> skb_dst_drop(skb);
> return sock_queue_rcv_skb(sk, skb);
> }
>
>
--
Ben Hutchings
Power corrupts. Absolute power is kind of neat.
- John Lehman, Secretary of the US Navy
1981-1987
signature.asc
Description: This is a digitally signed message part
Re: PROBLEM: null-ptr deref in ip_options_echo may lead to denial of service
On Tue, 2017-03-21 at 00:33 +, Ben Hutchings wrote:
> On Sun, 2017-03-19 at 22:25 -0700, Eric Dumazet wrote:
> > On Mon, 2017-03-20 at 12:59 +0800, Anarcheuz Fritz wrote:
> > > Hi David,
> > >
> > >
> > > While working on some legacy kernel I stumbled upon a null-ptr deref in
> > > ip_options_echo. The bug has been verified on the latest version
> > > 3.2.87 from the supported long-term branch.
> > >
> >
> > Fixed in commit 34b2cef20f19c87999fff3da4071e66937db9644
> > ("ipv4: keep skb->dst around in presence of IP options")
> >
> > For 3.2, since d826eb14ecef was not backported, following patch should
> > do it.
> >
> > (Bug origin was f84af32cbca70 ("net: ip_queue_rcv_skb() helper"))
>
> I see, I thought the vulnerability was introduced by d826eb14ecef.
>
> > diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> > index b3648bbef0da..a6e1eeb02267 100644
> > --- a/net/ipv4/ip_sockglue.c
> > +++ b/net/ipv4/ip_sockglue.c
> > @@ -1009,7 +1009,8 @@ e_inval:
> > */
> > int ip_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > {
> > - if (!(inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO))
> > + if (!(inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO) &&
> > + !IPCB(skb)->opt.optlen)
> > skb_dst_drop(skb);
> > return sock_queue_rcv_skb(sk, skb);
> > }
>
> Thanks to both of you; I'll queue this up for 3.2.
Actually, could I have a Signed-off-by for this, Eric?
Ben.
--
Ben Hutchings
Power corrupts. Absolute power is kind of neat.
- John Lehman, Secretary of the US Navy
1981-1987
signature.asc
Description: This is a digitally signed message part
Re: PROBLEM: null-ptr deref in ip_options_echo may lead to denial of service
On Sun, 2017-03-19 at 22:25 -0700, Eric Dumazet wrote:
> On Mon, 2017-03-20 at 12:59 +0800, Anarcheuz Fritz wrote:
> > Hi David,
> >
> >
> > While working on some legacy kernel I stumbled upon a null-ptr deref in
> > ip_options_echo. The bug has been verified on the latest version
> > 3.2.87 from the supported long-term branch.
> >
>
> Fixed in commit 34b2cef20f19c87999fff3da4071e66937db9644
> ("ipv4: keep skb->dst around in presence of IP options")
>
> For 3.2, since d826eb14ecef was not backported, following patch should
> do it.
>
> (Bug origin was f84af32cbca70 ("net: ip_queue_rcv_skb() helper"))
I see, I thought the vulnerability was introduced by d826eb14ecef.
> diff --git a/net/ipv4/ip_sockglue.c b/net/ipv4/ip_sockglue.c
> index b3648bbef0da..a6e1eeb02267 100644
> --- a/net/ipv4/ip_sockglue.c
> +++ b/net/ipv4/ip_sockglue.c
> @@ -1009,7 +1009,8 @@ e_inval:
> */
> int ip_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> {
> - if (!(inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO))
> + if (!(inet_sk(sk)->cmsg_flags & IP_CMSG_PKTINFO) &&
> + !IPCB(skb)->opt.optlen)
> skb_dst_drop(skb);
> return sock_queue_rcv_skb(sk, skb);
> }
Thanks to both of you; I'll queue this up for 3.2.
Ben.
--
Ben Hutchings
Power corrupts. Absolute power is kind of neat.
- John Lehman, Secretary of the US Navy
1981-1987
signature.asc
Description: This is a digitally signed message part
[PATCH 3.16 316/370] VSOCK: do not disconnect socket when peer has shutdown SEND only
3.16.42-rc1 review patch. If anyone has any objections, please let me know.
--
From: Ian Campbell
[ Upstream commit dedc58e067d8c379a15a8a183c5db318201295bb ]
The peer may be expecting a reply having sent a request and then done a
shutdown(SHUT_WR), so tearing down the whole socket at this point seems
wrong and breaks for me with a client which does a SHUT_WR.
Looking at other socket family's stream_recvmsg callbacks doing a shutdown
here does not seem to be the norm and removing it does not seem to have
had any adverse effects that I can see.
I'm using Stefan's RFC virtio transport patches, I'm unsure of the impact
on the vmci transport.
Signed-off-by: Ian Campbell
Cc: "David S. Miller"
Cc: Stefan Hajnoczi
Cc: Claudio Imbrenda
Cc: Andy King
Cc: Dmitry Torokhov
Cc: Jorgen Hansen
Cc: Adit Ranadive
Cc: netdev@vger.kernel.org
Signed-off-by: David S. Miller
Signed-off-by: Ben Hutchings
---
net/vmw_vsock/af_vsock.c | 21 +
1 file changed, 1 insertion(+), 20 deletions(-)
--- a/net/vmw_vsock/af_vsock.c
+++ b/net/vmw_vsock/af_vsock.c
@@ -1796,27 +1796,8 @@ vsock_stream_recvmsg(struct kiocb *kiocb
else if (sk->sk_shutdown & RCV_SHUTDOWN)
err = 0;
- if (copied > 0) {
- /* We only do these additional bookkeeping/notification steps
-* if we actually copied something out of the queue pair
-* instead of just peeking ahead.
-*/
-
- if (!(flags & MSG_PEEK)) {
- /* If the other side has shutdown for sending and there
-* is nothing more to read, then modify the socket
-* state.
-*/
- if (vsk->peer_shutdown & SEND_SHUTDOWN) {
- if (vsock_stream_has_data(vsk) <= 0) {
- sk->sk_state = SS_UNCONNECTED;
- sock_set_flag(sk, SOCK_DONE);
- sk->sk_state_change(sk);
- }
- }
- }
+ if (copied > 0)
err = copied;
- }
out_wait:
finish_wait(sk_sleep(sk), &wait);
[PATCH 3.16 330/370] net: sctp, forbid negative length
3.16.42-rc1 review patch. If anyone has any objections, please let me know.
--
From: Jiri Slaby
[ Upstream commit a4b8e71b05c27bae6bad3bdecddbc6b68a3ad8cf ]
Most of getsockopt handlers in net/sctp/socket.c check len against
sizeof some structure like:
if (len < sizeof(int))
return -EINVAL;
On the first look, the check seems to be correct. But since len is int
and sizeof returns size_t, int gets promoted to unsigned size_t too. So
the test returns false for negative lengths. Yes, (-1 < sizeof(long)) is
false.
Fix this in sctp by explicitly checking len < 0 before any getsockopt
handler is called.
Note that sctp_getsockopt_events already handled the negative case.
Since we added the < 0 check elsewhere, this one can be removed.
If not checked, this is the result:
UBSAN: Undefined behaviour in ../mm/page_alloc.c:2722:19
shift exponent 52 is too large for 32-bit type 'int'
CPU: 1 PID: 24535 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014
88006d99f2a8 b2f7bdea 41b58ab3
b4363c14 b2f7bcde 88006d99f2d0 88006d99f270
0034 b5096422
Call Trace:
[] ? __ubsan_handle_shift_out_of_bounds+0x29c/0x300
...
[] ? kmalloc_order+0x24/0x90
[] ? kmalloc_order_trace+0x24/0x220
[] ? __kmalloc+0x330/0x540
[] ? sctp_getsockopt_local_addrs+0x174/0xca0 [sctp]
[] ? sctp_getsockopt+0x10d/0x1b0 [sctp]
[] ? sock_common_getsockopt+0xb9/0x150
[] ? SyS_getsockopt+0x1a5/0x270
Signed-off-by: Jiri Slaby
Cc: Vlad Yasevich
Cc: Neil Horman
Cc: "David S. Miller"
Cc: linux-s...@vger.kernel.org
Cc: netdev@vger.kernel.org
Acked-by: Neil Horman
Signed-off-by: David S. Miller
Signed-off-by: Ben Hutchings
---
net/sctp/socket.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4280,7 +4280,7 @@ static int sctp_getsockopt_disable_fragm
static int sctp_getsockopt_events(struct sock *sk, int len, char __user
*optval,
int __user *optlen)
{
- if (len <= 0)
+ if (len == 0)
return -EINVAL;
if (len > sizeof(struct sctp_event_subscribe))
len = sizeof(struct sctp_event_subscribe);
@@ -5801,6 +5801,9 @@ static int sctp_getsockopt(struct sock *
if (get_user(len, optlen))
return -EFAULT;
+ if (len < 0)
+ return -EINVAL;
+
lock_sock(sk);
switch (optname) {
[PATCH 3.2 167/199] net: sctp, forbid negative length
3.2.87-rc1 review patch. If anyone has any objections, please let me know.
--
From: Jiri Slaby
[ Upstream commit a4b8e71b05c27bae6bad3bdecddbc6b68a3ad8cf ]
Most of getsockopt handlers in net/sctp/socket.c check len against
sizeof some structure like:
if (len < sizeof(int))
return -EINVAL;
On the first look, the check seems to be correct. But since len is int
and sizeof returns size_t, int gets promoted to unsigned size_t too. So
the test returns false for negative lengths. Yes, (-1 < sizeof(long)) is
false.
Fix this in sctp by explicitly checking len < 0 before any getsockopt
handler is called.
Note that sctp_getsockopt_events already handled the negative case.
Since we added the < 0 check elsewhere, this one can be removed.
If not checked, this is the result:
UBSAN: Undefined behaviour in ../mm/page_alloc.c:2722:19
shift exponent 52 is too large for 32-bit type 'int'
CPU: 1 PID: 24535 Comm: syz-executor Not tainted 4.8.1-0-syzkaller #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.9.1-0-gb3ef39f-prebuilt.qemu-project.org 04/01/2014
88006d99f2a8 b2f7bdea 41b58ab3
b4363c14 b2f7bcde 88006d99f2d0 88006d99f270
0034 b5096422
Call Trace:
[] ? __ubsan_handle_shift_out_of_bounds+0x29c/0x300
...
[] ? kmalloc_order+0x24/0x90
[] ? kmalloc_order_trace+0x24/0x220
[] ? __kmalloc+0x330/0x540
[] ? sctp_getsockopt_local_addrs+0x174/0xca0 [sctp]
[] ? sctp_getsockopt+0x10d/0x1b0 [sctp]
[] ? sock_common_getsockopt+0xb9/0x150
[] ? SyS_getsockopt+0x1a5/0x270
Signed-off-by: Jiri Slaby
Cc: Vlad Yasevich
Cc: Neil Horman
Cc: "David S. Miller"
Cc: linux-s...@vger.kernel.org
Cc: netdev@vger.kernel.org
Acked-by: Neil Horman
Signed-off-by: David S. Miller
[bwh: Backported to 3.2: adjust context]
Signed-off-by: Ben Hutchings
---
net/sctp/socket.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -4193,7 +4193,7 @@ static int sctp_getsockopt_disable_fragm
static int sctp_getsockopt_events(struct sock *sk, int len, char __user
*optval,
int __user *optlen)
{
- if (len <= 0)
+ if (len == 0)
return -EINVAL;
if (len > sizeof(struct sctp_event_subscribe))
len = sizeof(struct sctp_event_subscribe);
@@ -5586,6 +5586,9 @@ SCTP_STATIC int sctp_getsockopt(struct s
if (get_user(len, optlen))
return -EFAULT;
+ if (len < 0)
+ return -EINVAL;
+
sctp_lock_sock(sk);
switch (optname) {
Re: Bug#855153: linux-image-4.9.0-1-amd64: kernel 4.9 does not check route protocol when deleting ipv6 routes
Please queue up: commit c2ed1880fd61a998e3ce40254a99a2ad000f1a7d Author: Mantas M Date: Fri Dec 16 10:30:59 2016 +0200 net: ipv6: check route protocol when deleting routes for stable. Ben. On Tue, 2017-02-14 at 19:52 +0100, Pascal Mathis wrote: > Package: src:linux > Version: 4.9.6-3 > Severity: important > Tags: ipv6 > > Dear Debian Kernel Maintainers, > > when trying to delete an IPv6 route with a specific route protocol field > in a kernel 4.9, the kernel does not actually check the protocol of the > route and just deletes all the routes that match the other attributes. > > This leads to various issues with routing daemons, for example BIRD. > E.g. when a BGP withdraw update is being received with a prefix matching > a kernel route, both "proto bird" and "proto kernel" get deleted. > > The only workaround is currently to maintain a manual blacklist in the > routing daemon, however a proper fix at kernel level would be definitely > appreciated. As a patch was already committed to the Linux Kernel Git > Repository, I recommend backporting into the Debian Stretch kernel, as > the patch is both overseeable and easy to implement. > > The bug itself can be easily reproduced on any Linux running kernel > version 4.9 or lower by executing these commands: > > ~$ ip -6 route add ff::/64 dev eth0 proto kernel > ~$ ip -6 route > (check the routing table, the newly added route is visible) > ~$ ip -6 route del ff::/64 proto boot > ~$ ip -6 route > (the route is gone, although it should still be there!) > > A link to the Git commit fixing this specific issue can be found at the > following URL and was already merged into kernel 4.10 since rc1: > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=c2ed1880fd61a998e3ce40254a99a2ad000f1a7d [...] -- Ben Hutchings Reality is just a crutch for people who can't handle science fiction. signature.asc Description: This is a digitally signed message part
Re: [PATCH net 2/4] rtl8150: Use heap buffers for all register access
On Mon, Feb 06, 2017 at 04:09:18PM +, David Laight wrote: > From: Ben Hutchings [...] > > + ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0), > > + RTL8150_REQ_GET_REGS, RTL8150_REQT_READ, > > + indx, 0, buf, size, 500); > > + if (ret > 0 && ret <= size) > > + memcpy(data, buf, ret); > > If ret > size something is horridly wrong. > Silently not updating the callers buffer at all cannot be right. Yes, it seems strange to check this. I originally wrote this as ret > 0, but then I checked the usbnet core and found __usbnet_read_cmd() has the second comparison as well. > > + kfree(buf); > > + return ret; > > I can't help feeling that it would be better to add a wrapper to > usb_control_msg() that does the kmalloc() and memcpy()s and > drop that into all the call sites. It might be. Right now I'm trying to patch up a bunch of regressions rather than argue over an API change. Ben. -- Ben Hutchings Experience is what causes a person to make new mistakes instead of old ones. signature.asc Description: Digital signature
[PATCH net 3/4] catc: Combine failure cleanup code in catc_probe()
Signed-off-by: Ben Hutchings
---
drivers/net/usb/catc.c | 33 +
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c
index 3daa41bdd4ea..985909eab72c 100644
--- a/drivers/net/usb/catc.c
+++ b/drivers/net/usb/catc.c
@@ -776,7 +776,7 @@ static int catc_probe(struct usb_interface *intf, const
struct usb_device_id *id
struct net_device *netdev;
struct catc *catc;
u8 broadcast[ETH_ALEN];
- int i, pktsz;
+ int i, pktsz, ret;
if (usb_set_interface(usbdev,
intf->altsetting->desc.bInterfaceNumber, 1)) {
@@ -811,12 +811,8 @@ static int catc_probe(struct usb_interface *intf, const
struct usb_device_id *id
if ((!catc->ctrl_urb) || (!catc->tx_urb) ||
(!catc->rx_urb) || (!catc->irq_urb)) {
dev_err(&intf->dev, "No free urbs available.\n");
- usb_free_urb(catc->ctrl_urb);
- usb_free_urb(catc->tx_urb);
- usb_free_urb(catc->rx_urb);
- usb_free_urb(catc->irq_urb);
- free_netdev(netdev);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto fail_free;
}
/* The F5U011 has the same vendor/product as the netmate but a device
version of 0x130 */
@@ -913,16 +909,21 @@ static int catc_probe(struct usb_interface *intf, const
struct usb_device_id *id
usb_set_intfdata(intf, catc);
SET_NETDEV_DEV(netdev, &intf->dev);
- if (register_netdev(netdev) != 0) {
- usb_set_intfdata(intf, NULL);
- usb_free_urb(catc->ctrl_urb);
- usb_free_urb(catc->tx_urb);
- usb_free_urb(catc->rx_urb);
- usb_free_urb(catc->irq_urb);
- free_netdev(netdev);
- return -EIO;
- }
+ ret = register_netdev(netdev);
+ if (ret)
+ goto fail_clear_intfdata;
+
return 0;
+
+fail_clear_intfdata:
+ usb_set_intfdata(intf, NULL);
+fail_free:
+ usb_free_urb(catc->ctrl_urb);
+ usb_free_urb(catc->tx_urb);
+ usb_free_urb(catc->rx_urb);
+ usb_free_urb(catc->irq_urb);
+ free_netdev(netdev);
+ return ret;
}
static void catc_disconnect(struct usb_interface *intf)
signature.asc
Description: Digital signature
[PATCH net 4/4] catc: Use heap buffer for memory size test
Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ben Hutchings
---
drivers/net/usb/catc.c | 25 ++---
1 file changed, 18 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/catc.c b/drivers/net/usb/catc.c
index 985909eab72c..0acc9b640419 100644
--- a/drivers/net/usb/catc.c
+++ b/drivers/net/usb/catc.c
@@ -776,7 +776,7 @@ static int catc_probe(struct usb_interface *intf, const
struct usb_device_id *id
struct net_device *netdev;
struct catc *catc;
u8 broadcast[ETH_ALEN];
- int i, pktsz, ret;
+ int pktsz, ret;
if (usb_set_interface(usbdev,
intf->altsetting->desc.bInterfaceNumber, 1)) {
@@ -840,15 +840,24 @@ static int catc_probe(struct usb_interface *intf, const
struct usb_device_id *id
catc->irq_buf, 2, catc_irq_done, catc, 1);
if (!catc->is_f5u011) {
+ u32 *buf;
+ int i;
+
dev_dbg(dev, "Checking memory size\n");
- i = 0x12345678;
- catc_write_mem(catc, 0x7a80, &i, 4);
- i = 0x87654321;
- catc_write_mem(catc, 0xfa80, &i, 4);
- catc_read_mem(catc, 0x7a80, &i, 4);
+ buf = kmalloc(4, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto fail_free;
+ }
+
+ *buf = 0x12345678;
+ catc_write_mem(catc, 0x7a80, buf, 4);
+ *buf = 0x87654321;
+ catc_write_mem(catc, 0xfa80, buf, 4);
+ catc_read_mem(catc, 0x7a80, buf, 4);
- switch (i) {
+ switch (*buf) {
case 0x12345678:
catc_set_reg(catc, TxBufCount, 8);
catc_set_reg(catc, RxBufCount, 32);
@@ -863,6 +872,8 @@ static int catc_probe(struct usb_interface *intf, const
struct usb_device_id *id
dev_dbg(dev, "32k Memory\n");
break;
}
+
+ kfree(buf);
dev_dbg(dev, "Getting MAC from SEEROM.\n");
signature.asc
Description: Digital signature
[PATCH net 2/4] rtl8150: Use heap buffers for all register access
Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Ben Hutchings
---
drivers/net/usb/rtl8150.c | 34 +++---
1 file changed, 27 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 95b7bd0d7abc..c81c79110cef 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -155,16 +155,36 @@ static const char driver_name [] = "rtl8150";
*/
static int get_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
{
- return usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
- RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
- indx, 0, data, size, 500);
+ void *buf;
+ int ret;
+
+ buf = kmalloc(size, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = usb_control_msg(dev->udev, usb_rcvctrlpipe(dev->udev, 0),
+ RTL8150_REQ_GET_REGS, RTL8150_REQT_READ,
+ indx, 0, buf, size, 500);
+ if (ret > 0 && ret <= size)
+ memcpy(data, buf, ret);
+ kfree(buf);
+ return ret;
}
-static int set_registers(rtl8150_t * dev, u16 indx, u16 size, void *data)
+static int set_registers(rtl8150_t * dev, u16 indx, u16 size, const void *data)
{
- return usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
- RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
- indx, 0, data, size, 500);
+ void *buf;
+ int ret;
+
+ buf = kmemdup(data, size, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
+
+ ret = usb_control_msg(dev->udev, usb_sndctrlpipe(dev->udev, 0),
+ RTL8150_REQ_SET_REGS, RTL8150_REQT_WRITE,
+ indx, 0, buf, size, 500);
+ kfree(buf);
+ return ret;
}
static void async_set_reg_cb(struct urb *urb)
signature.asc
Description: Digital signature
[PATCH net 1/4] pegasus: Use heap buffers for all register access
Allocating USB buffers on the stack is not portable, and no longer
works on x86_64 (with VMAP_STACK enabled as per default).
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
References: https://bugs.debian.org/852556
Reported-by: Lisandro Damián Nicanor Pérez Meyer
Tested-by: Lisandro Damián Nicanor Pérez Meyer
Signed-off-by: Ben Hutchings
---
drivers/net/usb/pegasus.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 24e803fe9a53..36674484c6fb 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -126,40 +126,61 @@ static void async_ctrl_callback(struct urb *urb)
static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void
*data)
{
+ u8 *buf;
int ret;
+ buf = kmalloc(size, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
+
ret = usb_control_msg(pegasus->usb, usb_rcvctrlpipe(pegasus->usb, 0),
PEGASUS_REQ_GET_REGS, PEGASUS_REQT_READ, 0,
- indx, data, size, 1000);
+ indx, buf, size, 1000);
if (ret < 0)
netif_dbg(pegasus, drv, pegasus->net,
"%s returned %d\n", __func__, ret);
+ else if (ret <= size)
+ memcpy(data, buf, ret);
+ kfree(buf);
return ret;
}
-static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void
*data)
+static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
+const void *data)
{
+ u8 *buf;
int ret;
+ buf = kmemdup(data, size, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
+
ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
PEGASUS_REQ_SET_REGS, PEGASUS_REQT_WRITE, 0,
- indx, data, size, 100);
+ indx, buf, size, 100);
if (ret < 0)
netif_dbg(pegasus, drv, pegasus->net,
"%s returned %d\n", __func__, ret);
+ kfree(buf);
return ret;
}
static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
{
+ u8 *buf;
int ret;
+ buf = kmemdup(&data, 1, GFP_NOIO);
+ if (!buf)
+ return -ENOMEM;
+
ret = usb_control_msg(pegasus->usb, usb_sndctrlpipe(pegasus->usb, 0),
PEGASUS_REQ_SET_REG, PEGASUS_REQT_WRITE, data,
- indx, &data, 1, 1000);
+ indx, buf, 1, 1000);
if (ret < 0)
netif_dbg(pegasus, drv, pegasus->net,
"%s returned %d\n", __func__, ret);
+ kfree(buf);
return ret;
}
signature.asc
Description: Digital signature
[PATCH net 0/4] Fix on-stack USB buffers
Allocating USB buffers on the stack is not portable, and no longer works on x86_64 (with VMAP_STACK enabled as per default). This series fixes all the instances I could find where USB networking drivers do that. Ben. Ben Hutchings (4): pegasus: Use heap buffers for all register access rtl8150: Use heap buffers for all register access catc: Combine failure cleanup code in catc_probe() catc: Use heap buffer for memory size test drivers/net/usb/catc.c| 56 --- drivers/net/usb/pegasus.c | 29 drivers/net/usb/rtl8150.c | 34 ++-- 3 files changed, 86 insertions(+), 33 deletions(-) signature.asc Description: Digital signature
Re: [PATCH net v2] tipc: check minimum bearer MTU
On Thu, 2016-12-01 at 12:02 +0100, Michal Kubecek wrote:
[...]
> +/* check if device MTU is sufficient for tipc headers */
> +static inline bool tipc_check_mtu(struct net_device *dev, unsigned int
> reserve)
> +{
> + if (dev->mtu >= TIPC_MIN_BEARER_MTU + reserve)
> + return false;
> + netdev_warn(dev, "MTU too low for tipc bearer\n");
> + return true;
> +}
[...]
The comment says "check if ... sufficient" but the return value
indicates the opposite. Could you make these consistent?
Other than that, this looks OK to me. I haven't tested any version as
I don't know how to use TIPC.
Ben.
--
Ben Hutchings
A free society is one where it is safe to be unpopular. - Adlai
Stevenson
signature.asc
Description: This is a digitally signed message part
Re: [PATCH net] tipc: check minimum bearer MTU
On Wed, 2016-11-30 at 11:24 +0100, Michal Kubecek wrote:
> On Wed, Nov 30, 2016 at 10:57:02AM +0100, Michal Kubecek wrote:
> > Qian Zhang (张谦) reported a potential socket buffer overflow in
> > tipc_msg_build() which is also known as CVE-2016-8632: due to
> > insufficient checks, a buffer overflow can occur if MTU is too short for
> > even tipc headers. As anyone can set device MTU in a user/net namespace,
> > this issue can be abused by a regular user.
> >
> > As agreed in the discussion on Ben Hutchings' original patch, we should
> > check the MTU at the moment a bearer is attached rather than for each
> > processed packet. We also need to repeat the check when bearer MTU is
> > adjusted to new device MTU. UDP case also needs a check to avoid
> > overflow when calculating bearer MTU.
> >
> > Fixes: b97bf3fd8f6a ("[TIPC] Initial merge")
> > Signed-off-by: Michal Kubecek
> > Reported-by: Qian Zhang (张谦)
>
> Self-NACK.
>
> Im sorry, while testing this, I overlooked that an attempt to change
> MTU of an underlying device to low value issues a warning but it
> succeeds anyway.
[...]
I'm not sure that TIPC should block the MTU change, anyway. For IPv4
and IPv6 we disable the protocol on a device if its MTU is reduced
below the minimum. I think TIPC should behave the same way.
Ben.
--
Ben Hutchings
Never attribute to conspiracy what can adequately be explained by
stupidity.
signature.asc
Description: This is a digitally signed message part
Re: linux kernel 4.6 and 4.7 from backports have bug in congestion module refcount
u_startup_entry+0x2be/0x360 > Nov 15 01:40:00 archive kernel: [] ? > start_secondary+0x151/0x190 > Nov 15 01:40:00 archive kernel: ---[ end trace cdbbaec80305a0cc ]--- > > Tracing the code I found that the issue comes from the usage of a tcp > congestion module. > I use sysctl -w net.ipv4.tcp_congestion_control=htcp, which is compiled as > module. Default congestion algo, cubic, is compiled inside kernel, not > module. > > When the warning is triggered, /sys/module/tcp_htcp/refcnt is -1, which is > something that should not happen. It is triggered from inside > tcp_v4_destroy_sock, when calling tcp_cleanup_congestion_control, which > then calls module_put. > > This doesn't happen in 4.5.0-0.bpo.2 (2016-05-13) so I guess there is a bug > introduced from 4.5 to 4.6 which still lives in 4.7 and decrements > reference count of htcp congestion module when it shouldn't. The issue is > only triggered under heavy load (the above mentioned server is a 10g SFP+ > server with more than 4 Gbps traffic at certain times of day). I didn't see any bug fixes relating to congestion control refcounts after 4.7, so I'm guessing this bug is still present upstream. Ben. > I will test with a different congestion module (tcp_highspeed) to see if > the issue is only htcp related or it is general issue of using congestion > tcp modules. > > Panagiotis Malakoudis -- Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once. signature.asc Description: This is a digitally signed message part
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Fri, 2016-10-21 at 14:57 +, Jon Maloy wrote: > > -Original Message- > > > > From: Ben Hutchings [mailto:b...@decadent.org.uk] > > Sent: Thursday, 20 October, 2016 12:40 > > > > To: Jon Maloy ; Ying Xue > > > > > > Cc: netdev@vger.kernel.org; Qian Zhang ; Eric > > > > > > Dumazet > > > > > > Subject: Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build() > > > > On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: > > [...] > > > > At this point we're about to copy INT_H_SIZE + mhsz bytes into the > > > > first fragment. If that's already limited to be less than or equal to > > > > MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if > > > > MAX_H_SIZE > > > > is the maximum value of mhsz, that won't be good enough. > > > > > > > > > > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger > > > than > > > > the biggest header we are actually using, which is MCAST_H_SIZE (==44 > > bytes). > > > INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have > > > an mtu > > > > < 84 bytes. > > > You won't find any interfaces or protocols that come even close to this > > > > limitation, so to me this test is redundant. > > > > But I can easily create such an interface: > > > > $ unshare -n -U -r > > # ip l set lo mtu 1 > > > > Ben. > > > It won't be very useful though. But I assume you mean it could be a > possible exploit, Exactly. > and I suspect a few other things would break both in TIPC and in > other stacks if you do anything like that. I think the solution to > this is not to fix all possible places in the code where this can go > wrong, but rather to have a generic test where we refuse to attach > bearers/interfaces offering an mtu < e.g. 1000 bytes. This can easily > be done in tipc_enable_l2_media(). Yes. Ben. -- Ben Hutchings One of the nice things about standards is that there are so many of them. signature.asc Description: This is a digitally signed message part
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 14:51 +, Jon Maloy wrote: [...] > > At this point we're about to copy INT_H_SIZE + mhsz bytes into the > > first fragment. If that's already limited to be less than or equal to > > MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE > > is the maximum value of mhsz, that won't be good enough. > > > MAX_H_SIZE is 60 bytes, but in practice you will never see an mhsz larger > than the biggest header we are actually using, which is MCAST_H_SIZE (==44 > bytes). > INT_H_SIZE is 40 bytes, so you are in reality testing for whether we have an > mtu < 84 bytes. > You won't find any interfaces or protocols that come even close to this > limitation, so to me this test is redundant. But I can easily create such an interface: $ unshare -n -U -r # ip l set lo mtu 1 Ben. -- Ben Hutchings Never put off till tomorrow what you can avoid all together. signature.asc Description: This is a digitally signed message part
Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
On Thu, 2016-10-20 at 17:30 +0800, Ying Xue wrote: > On 10/19/2016 10:16 AM, Ben Hutchings wrote: > > Qian Zhang (张谦) reported a potential socket buffer overflow in > > tipc_msg_build(). The minimum fragment length needs to be checked > > against the maximum packet size, which is based on the link MTU. [...] > > > > --- a/net/tipc/msg.c > > +++ b/net/tipc/msg.c > > @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct > > msghdr *m, > > > > goto error; > > > > } > > > > > > + /* Check that fragment and message header will fit */ > > > > + if (INT_H_SIZE + mhsz > pktmax) > > + return -EMSGSIZE; > > > The "mhsz" represents the size of tipc packet header for current socket, > INT_H_SIZE indicates the size of tipc internal message header. So it > seems unreasonable to identify whether the sum of both header sizes is > bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to > compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should > return EMSGSIZE error code. At this point we're about to copy INT_H_SIZE + mhsz bytes into the first fragment. If that's already limited to be less than or equal to MAX_H_SIZE, comparing with MAX_H_SIZE would be fine. But if MAX_H_SIZE is the maximum value of mhsz, that won't be good enough. Ben. > > + > > > > /* Prepare reusable fragment header */ > > > > tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER, > > > > FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr)); > > > > > -- Ben Hutchings Never put off till tomorrow what you can avoid all together. signature.asc Description: This is a digitally signed message part
[PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
Qian Zhang (张谦) reported a potential socket buffer overflow in tipc_msg_build(). The minimum fragment length needs to be checked against the maximum packet size, which is based on the link MTU. Reported-by: Qian Zhang (张谦) Signed-off-by: Ben Hutchings --- This is untested, but I think it fixes the issue reported. Ideally tipc_l2_device_event() would also disable use of TIPC on devices with too small an MTU, like several other protocols do. Ben. net/tipc/msg.c | 4 1 file changed, 4 insertions(+) diff --git a/net/tipc/msg.c b/net/tipc/msg.c index 17201aa8423d..b9124ac82c29 100644 --- a/net/tipc/msg.c +++ b/net/tipc/msg.c @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m, goto error; } + /* Check that fragment and message header will fit */ + if (INT_H_SIZE + mhsz > pktmax) + return -EMSGSIZE; + /* Prepare reusable fragment header */ tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER, FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr)); signature.asc Description: Digital signature
Re: [PATCH V2] rtl_bt: Add firmware and config file for RTL8822BE
On Tue, 2016-08-30 at 20:11 -0500, Larry Finger wrote: > This device is a new model from Realtek. Updates to driver btrtl will > soon be submitted to the kernel. > > These files were provided by the Realtek developer. > > Signed-off-by: 陆朱伟 > Signed-off-by: Larry Finger > Cc: linux-blueto...@vger.kernel.org > --- > > V2 - fix error in file names in WHENCE > --- [...] > Found in vendor driver, linux_bt_usb_2.11.20140423_8723be.rar > From https://github.com/troy-tan/driver_store > +Files rtl_bt/rtl8822e_* came directly from Realtek. [...] You missed this wildcard, but I fixed it up. Applied and pushed, thanks. Ben. -- Ben Hutchings Anthony's Law of Force: Don't force it, get a larger hammer. signature.asc Description: This is a digitally signed message part
Re: [PATCH] rtl_bt: Add firmware and config file for RTL8822BE
On Tue, 2016-08-30 at 09:08 -0500, Larry Finger wrote: > This device is a new model from Realtek. Updates to driver btrtl will > soon be submitted to the kernel. > > These files were provided by the Realtek developer. > > Signed-off-by: 陆朱伟 > Signed-off-by: Larry Finger > Cc: linux-blueto...@vger.kernel.org > --- > WHENCE | 3 +++ > rtl_bt/rtl8822b_config.bin | Bin 0 -> 32 bytes > rtl_bt/rtl8822b_fw.bin | Bin 0 -> 51756 bytes > 3 files changed, 3 insertions(+) > create mode 100644 rtl_bt/rtl8822b_config.bin > create mode 100644 rtl_bt/rtl8822b_fw.bin > > diff --git a/WHENCE b/WHENCE > index d0bef0d..a9d7c97 100644 > --- a/WHENCE > +++ b/WHENCE > @@ -2755,11 +2755,14 @@ File: rtl_bt/rtl8723b_fw.bin > File: rtl_bt/rtl8761a_fw.bin > File: rtl_bt/rtl8812ae_fw.bin > File: rtl_bt/rtl8821a_fw.bin > +File: rtl_bt/rtl8822e_fw.bin > +File: rtl_bt/rtl8822e_config.bin [...] Should the filenames begin with "rtl822b" or "rtl822e"? Ben. -- Ben Hutchings All the simple programs have been written, and all the good names taken. signature.asc Description: This is a digitally signed message part
Re: CVE-2014-9900 fix is not upstream
On Tue, 2016-08-23 at 09:40 -0700, David Miller wrote: > From: Luis Henriques > Date: Tue, 23 Aug 2016 14:41:07 +0100 > > > Digging through some old CVEs I came across this one that doesn't > seem be > > in mainline. Was there a good reason for not being sent upstream? > Maybe it was > > rejected for some reason and I failed to find the discussion. > > Because the patch is completely bogus, and thus so is the CVE. > > The variable initializer clears out the entire structure. > > Until you can show compiler output from gcc that shows it not > initializing the structure I will not apply this patch because I know > that it faithfully does. On some versions and architectures. Can you guarantee that you will notice when an exception appears? Ben. -- Ben Hutchings The program is absolutely right; therefore, the computer must be wrong. signature.asc Description: This is a digitally signed message part
Re: net: Zeroing the structure ethtool_wolinfo in ethtool_get_wol()
On Tue, 2016-08-23 at 07:21 -0700, Eric Dumazet wrote:
> On Tue, 2016-08-23 at 14:41 +0100, Luis Henriques wrote:
> >
> > > > From: Avijit Kanti Das
> >
> > memset() the structure ethtool_wolinfo that has padded bytes
> > but the padded bytes have not been zeroed out.
> >
> > Change-Id: If3fd2d872a1b1ab9521d937b86a29fc468a8bbfe
> > > > Signed-off-by: Avijit Kanti Das
> > ---
> > net/core/ethtool.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> > index 977489820eb9..6bf6362e8114 100644
> > --- a/net/core/ethtool.c
> > +++ b/net/core/ethtool.c
> > @@ -1435,11 +1435,13 @@ static int ethtool_reset(struct net_device *dev,
> > char __user *useraddr)
> >
> > static int ethtool_get_wol(struct net_device *dev, char __user *useraddr)
> > {
> > - struct ethtool_wolinfo wol = { .cmd = ETHTOOL_GWOL };
> > + struct ethtool_wolinfo wol;
> >
> > if (!dev->ethtool_ops->get_wol)
> > return -EOPNOTSUPP;
> >
> > + memset(&wol, 0, sizeof(struct ethtool_wolinfo));
> > + wol.cmd = ETHTOOL_GWOL;
> > dev->ethtool_ops->get_wol(dev, &wol);
> >
> > if (copy_to_user(useraddr, &wol, sizeof(wol)))
>
> This would suggest a compiler bug to me.
Unfortunately the C standard does not guarantee that padding bytes are
initialised (at least not for automatic storage).
[...]
> If we can not rely on such constructs, we have hundreds of similar
> patches to submit.
[...]
Many such patches have been applied and can be found with:
git log --author=kangji...@gmail.com
Ben.
--
Ben Hutchings
The program is absolutely right; therefore, the computer must be wrong.
signature.asc
Description: This is a digitally signed message part
Re: [net-next PATCH] ethtool: Add support for SCTP verification tag in Rx NFC
On Sat, 2016-08-20 at 18:56 -0700, Alexander Duyck wrote: > > On Sat, Aug 20, 2016 at 5:21 PM, Ben Hutchings wrote: > > > > On Fri, 2016-08-19 at 14:32 -0700, Alexander Duyck wrote: > > > > > > The i40e hardware has support for SCTP filtering via Rx NFC however the > > > default configuration expects us to include the verification tag as a part > > > of the filter. In order to support that I need to be able to transfer > > > that > > > data through the ethtool interface via a new structure. > > > > > > This patch adds a new structure to allow us to pass the verification tag > > > for IPv4 or IPv6 SCTP traffic. > > [...] > > > > This looks like an incompatible ABI change. I suppose it could be OK > > if no drivers implemented flow steering for SCTP using the previously > > specified structure, but have you checked that that is the case? > > > > Ben. > > Well the structure itself matches the TCP flow spec for the TCP flow > sized portion. All I am doing with this patch is adding an extension > to that which is still confined to the 52 byte limit of the flow > > union. But that extension will be ignored by any drivers that implemented the API as previously defined. (If there aren't any, as I said, this doesn't really matter.) With previous extensions (everything in struct ethtool_flow_ext) we've introduced new type flags to ensure that they won't be silently ignored. You could add a new extended-SCTP type value for the same reason. [...] > One thing I could do if you would like would be to spin up another > patch to force the kernel to return -EINVAL if we are masking in > fields that are out of bounds for the flow specification. That way we > can handle this a bit more concisely in the future should we end up > > having to extend any other flow specifications. It's too late to do that now. Ben. -- Ben Hutchings Quantity is no substitute for quality, but it's the only one we've got. signature.asc Description: This is a digitally signed message part
Re: [net-next PATCH] ethtool: Add support for SCTP verification tag in Rx NFC
On Fri, 2016-08-19 at 14:32 -0700, Alexander Duyck wrote: > The i40e hardware has support for SCTP filtering via Rx NFC however the > default configuration expects us to include the verification tag as a part > of the filter. In order to support that I need to be able to transfer that > data through the ethtool interface via a new structure. > > This patch adds a new structure to allow us to pass the verification tag > for IPv4 or IPv6 SCTP traffic. [...] This looks like an incompatible ABI change. I suppose it could be OK if no drivers implemented flow steering for SCTP using the previously specified structure, but have you checked that that is the case? Ben. -- Ben Hutchings It's easier to fight for one's principles than to live up to them. signature.asc Description: This is a digitally signed message part
Re: [PATCH net-next V2] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)
On Thu, 2016-07-07 at 08:58 +0300, Netanel Belgazal wrote:
[...]
> +int ena_get_sset_count(struct net_device *netdev, int sset)
> +{
> + if (sset != ETH_SS_STATS)
> + return -EOPNOTSUPP;
> +
> + return netdev->num_tx_queues *
> + (ENA_STATS_ARRAY_TX + ENA_STATS_ARRAY_RX) +
> + ENA_STATS_ARRAY_GLOBAL + ENA_STATS_ARRAY_ENA_COM;
num_tx_queues is the number of software queues originally allocated,
which may be larger than the number in use (real_num_tx_queues).
And when actually generating the strings:
[...]
> +static void ena_queue_strings(struct ena_adapter *adapter, u8 **data)
> +{
> + const struct ena_stats *ena_stats;
> + int i, j;
> +
> + for (i = 0; i < adapter->num_queues; i++) {
you use adapter->num_queues.
[...]
> +static int ena_nway_reset(struct net_device *netdev)
> +{
> + return -ENODEV;
> +}
That doesn't make sense. The device is there but presumably doesn't
support autonegotiation. So don't implement the operation.
[...]
> +static int ena_get_rxfh(struct net_device *netdev, u32 *indir, u8 *key,
> + u8 *hfunc)
> +{
> + struct ena_adapter *adapter = netdev_priv(netdev);
> + enum ena_admin_hash_functions ena_func;
> + u8 func;
> + int rc;
> +
> + rc = ena_com_indirect_table_get(adapter->ena_dev, indir);
> + if (rc)
> + return rc;
> +
> + rc = ena_com_get_hash_function(adapter->ena_dev, &ena_func, key);
> + if (rc)
> + return rc;
> +
> + switch (ena_func) {
> + case ENA_ADMIN_TOEPLITZ:
> + func = ETH_RSS_HASH_TOP;
> + case ENA_ADMIN_CRC32:
> + func = ETH_RSS_HASH_XOR;
Missing break statements.
> + default:
> + netif_err(adapter, drv, netdev,
> + "Command parameter is not supported\n");
> + return -EOPNOTSUPP;
> + }
[...]
> +static const struct ethtool_ops ena_ethtool_ops = {
> + .get_settings = ena_get_settings,
[...]
get_settings is now deprecated in favour of get_link_ksettings.
Ben.
Ben.
--
Ben Hutchings
For every action, there is an equal and opposite criticism. - Harrison
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 2/2] net: ethernet: bcmgenet: use phy_ethtool_{get|set}_link_ksettings
On Tue, 2016-07-05 at 14:15 -0700, Florian Fainelli wrote:
> On 07/05/2016 02:07 PM, Philippe Reynes wrote:
> > Hi Florian,
> >
> > On 05/07/16 06:30, Florian Fainelli wrote:
> > > Le 04/07/2016 16:03, David Miller a écrit :
> > > > From: Philippe Reynes
> > > > Date: Sun, 3 Jul 2016 17:33:57 +0200
> > > >
> > > > > There are two generics functions phy_ethtool_{get|set}_link_ksettings,
> > > > > so we can use them instead of defining the same code in the driver.
> > > > >
> > > > > Signed-off-by: Philippe Reynes
> > > >
> > > > Applied.
> > > >
> > >
> > > The transformation is not equivalent, we lost the checks on
> > > netif_running() in the process, and those are here for a reason, if the
> > > interface is down and therefore clock gated, MDIO accesses to the PHY
> > > will simply fail outright and cause bus errors.
> >
> > Oh, I see, I've missed this. Sorry for this mistake.
> > We should revert this path.
>
> Well, maybe better than that, actually put the check in the generic
> functions, because if the link is down, aka netif_running() returns
> false, link parameters cannot be reliably queried and they are invalid.
Either the hardware or the driver needs to remember:
- Is auto-negotiation enabled
- If so, which modes are advertised
- If not, which mode is forced
And it should still be possible to get or set that information when the
interface is down.
Ben.
--
Ben Hutchings
Life is what happens to you while you're busy making other plans.
- John
Lennon
signature.asc
Description: This is a digitally signed message part
Re: ethtool needs a new maintainer
On Fri, 2016-07-01 at 10:33 +0800, zhuyj wrote: > I am interested in this, too. You didn't say anything about your experience with Linux networking, and I couldn't find many contributions from you. So you are welcome to help with ethtool but I've selected John as the maintainer. Ben. -- Ben Hutchings Lowery's Law: If it jams, force it. If it breaks, it needed replacing anyway. signature.asc Description: This is a digitally signed message part
Re: ethtool needs a new maintainer
On Thu, 2016-06-30 at 14:37 -0500, Jorge Alberto Garcia wrote: > El 30/06/2016 02:32 p.m., "Ben Hutchings" escribió: > > > > On Thu, 2016-06-30 at 14:27 -0500, Jorge Alberto Garcia wrote: > > > On Thu, Jun 30, 2016 at 1:15 PM, John W. Linville > > > wrote: > > > > On Mon, Jun 27, 2016 at 09:51:47AM -0400, John W. Linville wrote: > > > > > On Sun, Jun 26, 2016 at 06:11:41PM +0200, Ben Hutchings wrote: > > > > > > I've become steadily less enthusiastic and less responsive as a > > > > > > maintainer over the past year or so. I no longer work on > networking > > > > > > regularly, so it takes a lot more time to get into the right > state of > > > > > > mind to think about ethtool code, while I have other demands on > my time > > > > > > that tend to take priority. > > > > > > > > > > > > So, I would like to find a new maintainer to take over as soon as > > > > > > possible. Ideally the new maintainer would have previous > contributions > > > > > > to ethtool and an existing account on kernel.org so that they can > push > > > > > > to the git repository and the home page. But neither of those is > > > > > > essential. Please reply if you're interested. > > > > > > > > > > I would like to take this responsibility. My previous contributions > > > > > to ethtool are meager, but I think my skills and interests are > suited > > > > > to the task. Plus, I already have a kernel.org account... :-) > > > > > > > > Are there any other takers? Or is this a done deal? > > > > > > > > > > hi guys !, any link to a bugzilla / patchwork ? > > > > There's nothing as organised as that, though it might be possible to > > add categories for ethtool on <https://bugzilla.kernel.org> and > > <https://patchwork.ozlabs.org/project/netdev/>. > > > > Ben. > > > I would like to help but it will be a first for me. > Maybe in shadow mode ? I'm sure John will appreciate additional reviewers for ethtool submissions. If he decides to use Bugzilla then you should also be able to subscribe to the ethtool component there. Ben. -- Ben Hutchings Lowery's Law: If it jams, force it. If it breaks, it needed replacing anyway. signature.asc Description: This is a digitally signed message part
Re: ethtool needs a new maintainer
On Mon, 2016-06-27 at 09:51 -0400, John W. Linville wrote: > On Sun, Jun 26, 2016 at 06:11:41PM +0200, Ben Hutchings wrote: > > I've become steadily less enthusiastic and less responsive as a > > maintainer over the past year or so. I no longer work on networking > > regularly, so it takes a lot more time to get into the right state of > > mind to think about ethtool code, while I have other demands on my time > > that tend to take priority. > > > > So, I would like to find a new maintainer to take over as soon as > > possible. Ideally the new maintainer would have previous contributions > > to ethtool and an existing account on kernel.org so that they can push > > to the git repository and the home page. But neither of those is > > essential. Please reply if you're interested. > > I would like to take this responsibility. My previous contributions > to ethtool are meager, but I think my skills and interests are suited > to the task. Plus, I already have a kernel.org account... :-) [...] It was a tough choice, but I'm pleased to accept your offer. :-) I'll ask for the permissions to be updated accordingly. Ben. -- Ben Hutchings Lowery's Law: If it jams, force it. If it breaks, it needed replacing anyway. signature.asc Description: This is a digitally signed message part
Re: ethtool needs a new maintainer
On Thu, 2016-06-30 at 14:27 -0500, Jorge Alberto Garcia wrote: > On Thu, Jun 30, 2016 at 1:15 PM, John W. Linville > wrote: > > On Mon, Jun 27, 2016 at 09:51:47AM -0400, John W. Linville wrote: > > > On Sun, Jun 26, 2016 at 06:11:41PM +0200, Ben Hutchings wrote: > > > > I've become steadily less enthusiastic and less responsive as a > > > > maintainer over the past year or so. I no longer work on networking > > > > regularly, so it takes a lot more time to get into the right state of > > > > mind to think about ethtool code, while I have other demands on my time > > > > that tend to take priority. > > > > > > > > So, I would like to find a new maintainer to take over as soon as > > > > possible. Ideally the new maintainer would have previous contributions > > > > to ethtool and an existing account on kernel.org so that they can push > > > > to the git repository and the home page. But neither of those is > > > > essential. Please reply if you're interested. > > > > > > I would like to take this responsibility. My previous contributions > > > to ethtool are meager, but I think my skills and interests are suited > > > to the task. Plus, I already have a kernel.org account... :-) > > > > Are there any other takers? Or is this a done deal? > > > > hi guys !, any link to a bugzilla / patchwork ? There's nothing as organised as that, though it might be possible to add categories for ethtool on <https://bugzilla.kernel.org> and <https://patchwork.ozlabs.org/project/netdev/>. Ben. -- Ben Hutchings To err is human; to really foul things up requires a computer. signature.asc Description: This is a digitally signed message part
Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics Information Support
On Mon, 2016-06-27 at 00:02 +0200, Ben Hutchings wrote: > On Sun, 2016-06-26 at 09:40 -0700, Vidya Sagar Ravipati wrote: > > On Sun, Jun 26, 2016 at 2:33 AM, Ben Hutchings wrote: > [...] > > > This looks very similar to sff8472_diags, only with the actual values > > > separated from the arrays of thresholds. > > > > > > Can the structure and code be combined with sfpdiag.c, with the > > > additional per-channel diagnostics being optional? > > > > Diagnostic dom information in QSFP has lot more information compared > > to SFPs and as part of this checkin , basic dom information in qsfp which is > > equivalent to sfp dom is getting exposed as part of this checkin. > > > > Here are list of fields (not complete) which are used for debugging QSFP > > issues, will be added for this structure in next patch sets > > a) TX/RX output amplitude conttrol > > b) TX_DISABLE > > b) TX_FAULT > > c) TX CDR > > d) RX CDR > > e) RX output disable > > f) Rate select option > > > > Please let me know if it make sense to maintain the different structure > > with above explanation or whether it is required to be combined. > [...] > > I think there's enough information in common that it does make sense to > use common reporting functions, and that in turn suggests that it would > make sense to use a common structure. You could alternately have the > callers in sfpdiag.c and qsfp.c extract the relevant fields and pass > them into the reporting functions. > > The substantial duplication of reporting code from sfpid.c in your > latest submission is not OK. I mean sfpdiag.c here. I didn't check for duplication from sfpid.c, but I think you've avoided that. Ben. -- Ben Hutchings Humour is the best antidote to reality. signature.asc Description: This is a digitally signed message part
Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics Information Support
On Sun, 2016-06-26 at 09:40 -0700, Vidya Sagar Ravipati wrote: > On Sun, Jun 26, 2016 at 2:33 AM, Ben Hutchings wrote: [...] > > This looks very similar to sff8472_diags, only with the actual values > > separated from the arrays of thresholds. > > > > Can the structure and code be combined with sfpdiag.c, with the > > additional per-channel diagnostics being optional? > > Diagnostic dom information in QSFP has lot more information compared > to SFPs and as part of this checkin , basic dom information in qsfp which is > equivalent to sfp dom is getting exposed as part of this checkin. > > Here are list of fields (not complete) which are used for debugging QSFP > issues, will be added for this structure in next patch sets > a) TX/RX output amplitude conttrol > b) TX_DISABLE > b) TX_FAULT > c) TX CDR > d) RX CDR > e) RX output disable > f) Rate select option > > Please let me know if it make sense to maintain the different structure > with above explanation or whether it is required to be combined. [...] I think there's enough information in common that it does make sense to use common reporting functions, and that in turn suggests that it would make sense to use a common structure. You could alternately have the callers in sfpdiag.c and qsfp.c extract the relevant fields and pass them into the reporting functions. The substantial duplication of reporting code from sfpid.c in your latest submission is not OK. Ben. -- Ben Hutchings compatible: Gracefully accepts erroneous data from any source signature.asc Description: This is a digitally signed message part
ethtool needs a new maintainer
I've become steadily less enthusiastic and less responsive as a maintainer over the past year or so. I no longer work on networking regularly, so it takes a lot more time to get into the right state of mind to think about ethtool code, while I have other demands on my time that tend to take priority. So, I would like to find a new maintainer to take over as soon as possible. Ideally the new maintainer would have previous contributions to ethtool and an existing account on kernel.org so that they can push to the git repository and the home page. But neither of those is essential. Please reply if you're interested. I was thinking of adding a TODO file to the repository, but it's really for the new maintainer to decide what to do. So here's my list as a suggestion: * Add regression test coverage for all sub-commands with complex logic * Internationalise output and error messages * Build a libethtool that handles all the API quirks and fallbacks for old kernel versions. This might help people writing language bindings or other utilities that use the ethtool API. * Provide a 'cleaned up' ethtool (under some other name) that has: - More conventional sub-command syntax, i.e. no '-'/'--' prefix - More consistent output formatting Ben. -- Ben Hutchings compatible: Gracefully accepts erroneous data from any source signature.asc Description: This is a digitally signed message part
ethtool 4.6 released
ethtool version 4.6 has been released. Home page: https://www.kernel.org/pub/software/network/ethtool/ Download link: https://www.kernel.org/pub/software/network/ethtool/ethtool-4.6.tar.xz Release notes: * Feature: Support register dump on Intel X550 NICs (-d option) * Fix: Correct some reported register offsets on Intel 10GbE NICs (-d option) * Feature: Add IPv6 support to NFC (-n, -N, -u and -U options) * Feature: Add support for ETHTOOL_xLINKSETTINGS ioctls (no option and -s option) * Feature: Use netlink socket when AF_INET not available Ben. -- Ben Hutchings compatible: Gracefully accepts erroneous data from any source signature.asc Description: This is a digitally signed message part
[PATCH ethtool 2/2] ethtool.8.in, ethtool.c: Add myself to authors and copyright notices
Signed-off-by: Ben Hutchings --- ethtool.8.in | 3 ++- ethtool.c| 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/ethtool.8.in b/ethtool.8.in index 9dc5e252a1dc..e08c9a714551 100644 --- a/ethtool.8.in +++ b/ethtool.8.in @@ -947,7 +947,8 @@ Scott Feldman, Andi Kleen, Alexander Duyck, Sucheta Chakraborty, -Jesse Brandeburg. +Jesse Brandeburg, +Ben Hutchings. .SH AVAILABILITY .B ethtool is available from diff --git a/ethtool.c b/ethtool.c index 093adf735793..4aa476264b3a 100644 --- a/ethtool.c +++ b/ethtool.c @@ -21,6 +21,8 @@ * MDI-X set support by Jesse Brandeburg * Copyright 2012 Intel Corporation * vmxnet3 support by Shrikrishna Khare + * Various features by Ben Hutchings ; + * Copyright 2008-2010, 2013-2016 Ben Hutchings * * TODO: * * show settings for all devices signature.asc Description: Digital signature
[PATCH ethtool 1/2] configure.ac: Remove feature test for
has been a hard dependency for as back as git history goes (2005, shortly after version 3). Signed-off-by: Ben Hutchings --- I spotted this while reviewing David Decotigny's patch to use netlink sockets. Ben. configure.ac | 1 - 1 file changed, 1 deletion(-) diff --git a/configure.ac b/configure.ac index 3105415be026..6ebffedc9f5b 100644 --- a/configure.ac +++ b/configure.ac @@ -15,7 +15,6 @@ AM_PROG_CC_C_O dnl Checks for libraries. dnl Checks for header files. -AC_CHECK_HEADERS(sys/ioctl.h) dnl Checks for typedefs, structures, and compiler characteristics. AC_MSG_CHECKING([whether defines big-endian types]) signature.asc Description: Digital signature
Re: [PATCH 2/5] ethtool: move cmdline_coalesce out of do_scoalesce
struct ethtool_coalesce ecoal;
> int gcoalesce_changed = 0;
> - s32 coal_stats_wanted = -1;
> - int coal_adaptive_rx_wanted = -1;
> - int coal_adaptive_tx_wanted = -1;
> - s32 coal_sample_rate_wanted = -1;
> - s32 coal_pkt_rate_low_wanted = -1;
> - s32 coal_pkt_rate_high_wanted = -1;
> - s32 coal_rx_usec_wanted = -1;
> - s32 coal_rx_frames_wanted = -1;
> - s32 coal_rx_usec_irq_wanted = -1;
> - s32 coal_rx_frames_irq_wanted = -1;
> - s32 coal_tx_usec_wanted = -1;
> - s32 coal_tx_frames_wanted = -1;
> - s32 coal_tx_usec_irq_wanted = -1;
> - s32 coal_tx_frames_irq_wanted = -1;
> - s32 coal_rx_usec_low_wanted = -1;
> - s32 coal_rx_frames_low_wanted = -1;
> - s32 coal_tx_usec_low_wanted = -1;
> - s32 coal_tx_frames_low_wanted = -1;
> - s32 coal_rx_usec_high_wanted = -1;
> - s32 coal_rx_frames_high_wanted = -1;
> - s32 coal_tx_usec_high_wanted = -1;
> - s32 coal_tx_frames_high_wanted = -1;
> - struct cmdline_info cmdline_coalesce[] = {
> - { "adaptive-rx", CMDL_BOOL, &coal_adaptive_rx_wanted,
> - &ecoal.use_adaptive_rx_coalesce },
> - { "adaptive-tx", CMDL_BOOL, &coal_adaptive_tx_wanted,
> - &ecoal.use_adaptive_tx_coalesce },
> - { "sample-interval", CMDL_S32, &coal_sample_rate_wanted,
> - &ecoal.rate_sample_interval },
> - { "stats-block-usecs", CMDL_S32, &coal_stats_wanted,
> - &ecoal.stats_block_coalesce_usecs },
> - { "pkt-rate-low", CMDL_S32, &coal_pkt_rate_low_wanted,
> - &ecoal.pkt_rate_low },
> - { "pkt-rate-high", CMDL_S32, &coal_pkt_rate_high_wanted,
> - &ecoal.pkt_rate_high },
> - { "rx-usecs", CMDL_S32, &coal_rx_usec_wanted,
> - &ecoal.rx_coalesce_usecs },
> - { "rx-frames", CMDL_S32, &coal_rx_frames_wanted,
> - &ecoal.rx_max_coalesced_frames },
> - { "rx-usecs-irq", CMDL_S32, &coal_rx_usec_irq_wanted,
> - &ecoal.rx_coalesce_usecs_irq },
> - { "rx-frames-irq", CMDL_S32, &coal_rx_frames_irq_wanted,
> - &ecoal.rx_max_coalesced_frames_irq },
> - { "tx-usecs", CMDL_S32, &coal_tx_usec_wanted,
> - &ecoal.tx_coalesce_usecs },
> - { "tx-frames", CMDL_S32, &coal_tx_frames_wanted,
> - &ecoal.tx_max_coalesced_frames },
> - { "tx-usecs-irq", CMDL_S32, &coal_tx_usec_irq_wanted,
> - &ecoal.tx_coalesce_usecs_irq },
> - { "tx-frames-irq", CMDL_S32, &coal_tx_frames_irq_wanted,
> - &ecoal.tx_max_coalesced_frames_irq },
> - { "rx-usecs-low", CMDL_S32, &coal_rx_usec_low_wanted,
> - &ecoal.rx_coalesce_usecs_low },
> - { "rx-frames-low", CMDL_S32, &coal_rx_frames_low_wanted,
> - &ecoal.rx_max_coalesced_frames_low },
> - { "tx-usecs-low", CMDL_S32, &coal_tx_usec_low_wanted,
> - &ecoal.tx_coalesce_usecs_low },
> - { "tx-frames-low", CMDL_S32, &coal_tx_frames_low_wanted,
> - &ecoal.tx_max_coalesced_frames_low },
> - { "rx-usecs-high", CMDL_S32, &coal_rx_usec_high_wanted,
> - &ecoal.rx_coalesce_usecs_high },
> - { "rx-frames-high", CMDL_S32, &coal_rx_frames_high_wanted,
> - &ecoal.rx_max_coalesced_frames_high },
> - { "tx-usecs-high", CMDL_S32, &coal_tx_usec_high_wanted,
> - &ecoal.tx_coalesce_usecs_high },
> - { "tx-frames-high", CMDL_S32, &coal_tx_frames_high_wanted,
> - &ecoal.tx_max_coalesced_frames_high },
> - };
> int err, changed = 0;
>
> parse_generic_cmdline(ctx, &gcoalesce_changed,
> cmdline_coalesce, ARRAY_SIZE(cmdline_coalesce));
>
> - ecoal.cmd = ETHTOOL_GCOALESCE;
> - err = send_ioctl(ctx, &ecoal);
> + s_ecoal.cmd = ETHTOOL_GCOALESCE;
> + err = send_ioctl(ctx, &s_ecoal);
> if (err) {
> perror("Cannot get device coalesce settings");
> return 76;
> @@ -1975,8 +1976,8 @@ static int do_scoalesce(struct cmd_context *ctx)
> return 80;
> }
>
> - ecoal.cmd = ETHTOOL_SCOALESCE;
> - err = send_ioctl(ctx, &ecoal);
> + s_ecoal.cmd = ETHTOOL_SCOALESCE;
> + err = send_ioctl(ctx, &s_ecoal);
> if (err) {
> perror("Cannot set device coalesce parameters");
> return 81;
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
signature.asc
Description: This is a digitally signed message part
Re: [ethtool PATCH v1 2/2] ethtool:QSFP Plus/QSFP28 Diagnostics Information Support
On Mon, 2016-06-13 at 16:27 -0700, Vidya Sagar Ravipati wrote:
> From: Vidya Sagar Ravipati
>
> This patch series provides following support
> a) Reorganized fields based out of SFF-8024 fields i.e. Identifier/
> Encoding/Connector types which are common across SFP/SFP+ (SFF-8472)
> and QSFP+/QSFP28 (SFF-8436/SFF-8636) modules into sff-common files.
> a) Support for diagnostics information for QSFP Plus/QSFP28 modules
> based on SFF-8436/SFF-8636
Those two changes should be separate patches.
> Standards for QSFP+/QSFP28
> a) QSFP+/QSFP28 - SFF 8636 Rev 2.7 dated January 26,2016
> b) SFF-8024 Rev 4.0 dated May 31, 2016
>
> Signed-off-by: Vidya Sagar Ravipati
> Acked-by: Bert Kenward
> ---
> Makefile.am | 2 +-
> ethtool.c| 5 +
> internal.h | 3 +
> qsfp.c | 876
> +++
> qsfp.h | 599
> sff-common.c | 255 +
> sff-common.h | 156 +++
> sfpid.c | 103 +--
> 8 files changed, 1899 insertions(+), 100 deletions(-)
> create mode 100644 qsfp.c
> create mode 100644 qsfp.h
> create mode 100644 sff-common.c
> create mode 100644 sff-common.h
>
> diff --git a/Makefile.am b/Makefile.am
> index 6814bc9..1f3d767 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -13,7 +13,7 @@ ethtool_SOURCES += \
> fec_8xx.c ibm_emac.c ixgb.c ixgbe.c natsemi.c \
> pcnet32.c realtek.c tg3.c marvell.c vioc.c\
> smsc911x.c at76c50x-usb.c sfc.c stmmac.c \
> - sfpid.c sfpdiag.c ixgbevf.c tse.c vmxnet3.c
> + sff-common.c sfpid.c sfpdiag.c ixgbevf.c tse.c vmxnet3.c
> qsfp.c
> endif
>
> TESTS = test-cmdline test-features
> diff --git a/ethtool.c b/ethtool.c
> index 0cd0d4f..a0d48d1 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -3963,6 +3963,11 @@ static int do_getmodule(struct cmd_context *ctx)
> sff8079_show_all(eeprom->data);
> sff8472_show_all(eeprom->data);
> break;
> + case ETH_MODULE_SFF_8436:
> + case ETH_MODULE_SFF_8636:
> + sff8636_show_all(eeprom->data,
> + modinfo.eeprom_len);
The last line here is wrongly indented, as are many other continuation
lines. ethtool code should be formatted just the same way David likes
kernel networking code.
[...]
> --- /dev/null
> +++ b/qsfp.c
[...]
> +/* Channel Monitoring Fields */
> +struct sff8636_channel_diags {
> +
> + __u16 bias_cur; /* Measured bias current in 2uA units */
> + __u16 rx_power; /* Measured RX Power */
> + __u16 tx_power; /* Measured TX Power */
> +
> +};
There's no need for blank lines in this structure definition.
> +/* Module Monitoring Fields */
> +struct sff8636_diags {
> +
> +#define MAX_CHANNEL_NUM 4
> +#define LWARN 0
> +#define HWARN 1
> +#define LALRM 2
> +#define HALRM 3
> +
> + /* Supports DOM */
> + __u8 supports_dom;
> + /* Supports alarm/warning thold */
> + __u8 supports_alarms;
> + /* RX Power: 0 = OMA, 1 = Average power */
> + __u8 rx_power_type;
> + /* TX Power: 0 = Not supported, 1 = Average power */
> + __u8 tx_power_type;
> + /* SFP voltage in 0.1mV units */
> + __u16 sfp_voltage;
> + /* SFP Temp in 16-bit signed 1/256 Celcius */
> + __s16 sfp_temp;
> + /* [4] tables are low/high warn, low/high alarm */
> + /* SFP voltage in 0.1mV units */
> + __u16 thresh_sfp_voltage[4];
> + /* SFP Temp in 16-bit signed 1/256 Celcius */
> + __s16 thresh_sfp_temp[4];
> + /* Measured bias current in 2uA units */
> + __u16 thresh_bias_cur[4];
> + /* Measured TX Power */
> + __u16 thresh_tx_power[4];
> + /* Measured RX Power */
> + __u16 thresh_rx_power[4];
This looks very similar to sff8472_diags, only with the actual values
separated from the arrays of thresholds.
Can the structure and code be combined with sfpdiag.c, with the
additional per-channel diagnostics being optional?
> + struct sff8636_channel_diags scd[MAX_CHANNEL_NUM];
> +};
[...]
> --- /dev/null
> +++ b/sff-common.c
[...]
> +double convert_mw_to_dbm(double mw)
> +{
> + return (10. * log10(mw / 1000.)) + 30.;
> +}
[...]
This is copied from sfpdiag.c, so you should make that file use it
rather than a duplicate definition.
Ben.
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
signature.asc
Description: This is a digitally signed message part
Re: [ethtool 0/3][pull request] Intel Wired LAN Driver Updates 2016-05-03
On Wed, 2016-05-04 at 09:44 -0700, Jeff Kirsher wrote: > This series contains updates to ixgbe in ethtool. > > Preethi adds missing device IDs and mac_type definitions, also updated > the display registers for x550, x550em_x/a. Cleaned up the format string > storage by taking advantage of "for" loops. I've pulled these. Thanks for your patience. Ben. > The following are changes since commit > deb1c6613ec14fd828d321e38c7bea45fe559bd5: > Release version 4.5. > and are available in the git repository at: > git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/ethtool master > > Preethi Banala (3): > ethtool/ixgbe: Add device ID and mac_type definitions > ethtool/ixgbe: Correct offsets and support x550, x550em_x, x550em_a > ethtool/ixgbe: Reduce format string storage > > ixgbe.c | 173 > +++--------- > 1 file changed, 95 insertions(+), 78 deletions(-) > -- Ben Hutchings compatible: Gracefully accepts erroneous data from any source signature.asc Description: This is a digitally signed message part
Re: [PATCH ethtool] ethtool.c: fix memory leaks
s !=
> ((old_state->off_flags & ~off_flags_mask) |
> @@ -2238,13 +2244,19 @@ static int do_sfeatures(struct cmd_context
> *ctx)
> if (!any_changed) {
> fprintf(stderr,
> "Could not change any device
> features\n");
> - return 1;
> + goto finish;
> }
> printf("Actual changes:\n");
> dump_features(defs, new_state, old_state);
> }
>
> - return 0;
> + retval = 0;
> +finish:
> + free(new_state);
> + free(old_state);
> + free(efeatures);
> + free(defs);
> + return retval;
> }
>
> static int do_gset(struct cmd_context *ctx)
> @@ -2705,8 +2717,18 @@ static int do_gregs(struct cmd_context *ctx)
> return 75;
> }
>
> - regs = realloc(regs, sizeof(*regs) + st.st_size);
> - regs->len = st.st_size;
> + if (regs->len != st.st_size) {
> + struct ethtool_regs *new_regs;
> + new_regs = realloc(regs, sizeof(*regs) +
> st.st_size);
> + if (!new_regs) {
> + perror("Cannot allocate memory for
> register "
> + "dump");
> + free(regs);
> + return 73;
> + }
> + regs = new_regs;
> + regs->len = st.st_size;
> + }
> nread = fread(regs->data, regs->len, 1, f);
> fclose(f);
> if (nread != 1) {
> @@ -3775,6 +3797,7 @@ static int do_gprivflags(struct cmd_context
> *ctx)
> }
> if (strings->len == 0) {
> fprintf(stderr, "No private flags defined\n");
> + free(strings);
> return 1;
> }
> if (strings->len > 32) {
> @@ -3786,6 +3809,7 @@ static int do_gprivflags(struct cmd_context
> *ctx)
> flags.cmd = ETHTOOL_GPFLAGS;
> if (send_ioctl(ctx, &flags)) {
> perror("Cannot get private flags");
> + free(strings);
> return 1;
> }
>
> @@ -3804,6 +3828,7 @@ static int do_gprivflags(struct cmd_context
> *ctx)
> (const char *)strings->data + i *
> ETH_GSTRING_LEN,
> (flags.data & (1U << i)) ? "on" : "off");
>
> + free(strings);
> return 0;
> }
>
> @@ -3825,6 +3850,7 @@ static int do_sprivflags(struct cmd_context
> *ctx)
> }
> if (strings->len == 0) {
> fprintf(stderr, "No private flags defined\n");
> + free(strings);
> return 1;
> }
> if (strings->len > 32) {
> @@ -3836,6 +3862,7 @@ static int do_sprivflags(struct cmd_context
> *ctx)
> cmdline = calloc(strings->len, sizeof(*cmdline));
> if (!cmdline) {
> perror("Cannot parse arguments");
> + free(strings);
> return 1;
> }
> for (i = 0; i < strings->len; i++) {
> @@ -3852,6 +3879,7 @@ static int do_sprivflags(struct cmd_context
> *ctx)
> flags.cmd = ETHTOOL_GPFLAGS;
> if (send_ioctl(ctx, &flags)) {
> perror("Cannot get private flags");
> + free(strings);
> return 1;
> }
>
> @@ -3859,9 +3887,11 @@ static int do_sprivflags(struct cmd_context
> *ctx)
> flags.data = (flags.data & ~seen_flags) | wanted_flags;
> if (send_ioctl(ctx, &flags)) {
> perror("Cannot set private flags");
> + free(strings);
> return 1;
> }
>
> + free(strings);
> return 0;
> }
>
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
signature.asc
Description: This is a digitally signed message part
Re: [ethtool PATCH v7 2/2] ethtool: use netlink socket when AF_INET not available
On Fri, 2016-03-25 at 09:21 -0700, David Decotigny wrote:
> From: David Decotigny
>
> To benefit from this, kernel commit 025c68186e07 ("netlink: add support
> for NIC driver ioctls") is needed.
>
>
> Signed-off-by: David Decotigny
> ---
> configure.ac | 2 +-
> ethtool.c| 7 +++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/configure.ac b/configure.ac
> index 3105415..47d2a0f 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -15,7 +15,7 @@ AM_PROG_CC_C_O
> dnl Checks for libraries.
>
> dnl Checks for header files.
> -AC_CHECK_HEADERS(sys/ioctl.h)
> +AC_CHECK_HEADERS(sys/ioctl.h linux/netlink.h)
This doesn't make a lot of sense. That header has been around since
Linux 2.1. But it didn't define NETLINK_GENERIC then.
(The test for doesn't make sense either, as we include it
unconditionally!)
> dnl Checks for typedefs, structures, and compiler characteristics.
> AC_MSG_CHECKING([whether defines big-endian types])
> diff --git a/ethtool.c b/ethtool.c
> index cb3d971..314b1b8 100644
> --- a/ethtool.c
> +++ b/ethtool.c
> @@ -42,6 +42,9 @@
> #include
>
> #include
> +#ifdef HAVE_LINUX_NETLINK_H
> +# include
> +#endif
>
So what I've committed instead makes this unconditional, but adds a
conditional definition of NETLINK_GENERIC.
Ben.
>
> #ifndef MAX_ADDR_LEN
> #define MAX_ADDR_LEN 32
> @@ -4645,6 +4648,10 @@ opt_found:
>
> /* Open control socket. */
> ctx.fd = socket(AF_INET, SOCK_DGRAM, 0);
> +#ifdef HAVE_LINUX_NETLINK_H
> + if (ctx.fd < 0)
> + ctx.fd = socket(AF_NETLINK, SOCK_RAW, NETLINK_GENERIC);
> +#endif
> if (ctx.fd < 0) {
> perror("Cannot get control socket");
> return 70;
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
signature.asc
Description: This is a digitally signed message part
Re: [ethtool PATCH v7 1/2] ethtool.c: add support for ETHTOOL_xLINKSETTINGS ioctls
On Fri, 2016-03-25 at 09:21 -0700, David Decotigny wrote:
> From: David Decotigny
>
> More info with kernel commit 8d3f2806f8fb ("Merge branch
> 'ethtool-ksettings'").
>
> Note: The new features implemented in this patch depend on kernel
> commit 793cf87de9d1 ("Set cmd field in ETHTOOL_GLINKSETTINGS response to
> wrong nwords").
[...]
Applied, with some style changes. Thanks for your patience.
Ben.
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v2 ethtool 0/2] IPv6 RXNFC
On Thu, 2016-03-17 at 19:09 +, Edward Cree wrote:
> This series adds support for steering of IPv6 receive flows.
>
> Changes since v1:
> * Fixed and simplified IPv6 address and mask parsing
> * Made help text / man page more consistent
> * Dropped ethtool-copy.h patch as upstream is now newer
Applied these two, and added the patch below. Thanks for your
patience.
Ben.
---
Change IP parameter syntax in documentation to just 'ip-address'
Writing out the IPv4 address syntax repeatedly is not really
necessary, and extending it to cover IPv6 addresses will just make the
documentation harder to read.
Signed-off-by: Ben Hutchings
---
ethtool.8.in | 2 +-
ethtool.c| 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/ethtool.8.in b/ethtool.8.in
index 36da10ed6c87..9dc5e252a1dc 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -54,7 +54,7 @@
.\"
.\"\(*PA - IP address
.\"
-.ds PA \fIx\fP\fB.\fP\fIx\fP\fB.\fP\fIx\fP\fB.\fP\fIx\fP
+.ds PA \fIip-address\fP
.\"
.\"\(*WO - wol flags
.\"
diff --git a/ethtool.c b/ethtool.c
index a92137f7f5b1..d04bf9fedafd 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -4163,8 +4163,8 @@ static const struct option {
" [ src %x:%x:%x:%x:%x:%x [m %x:%x:%x:%x:%x:%x]
]\n"
" [ dst %x:%x:%x:%x:%x:%x [m %x:%x:%x:%x:%x:%x]
]\n"
" [ proto %d [m %x] ]\n"
- " [ src-ip %d.%d.%d.%d [m %d.%d.%d.%d] ]\n"
- " [ dst-ip %d.%d.%d.%d [m %d.%d.%d.%d] ]\n"
+ " [ src-ip IP-ADDRESS [m IP-ADDRESS] ]\n"
+ " [ dst-ip IP-ADDRESS [m IP-ADDRESS] ]\n"
" [ tos %d [m %x] ]\n"
" [ tclass %d [m %x] ]\n"
" [ l4proto %d [m %x] ]\n"
--
Ben Hutchings
compatible: Gracefully accepts erroneous data from any source
signature.asc
Description: This is a digitally signed message part
[PATCH net-next] of_mdio: Enable fixed PHY support if driver is a module
The fixed_phy driver doesn't have to be built-in, and it's important that of_mdio supports it even if it's a module. Signed-off-by: Ben Hutchings --- Re-sending with the proper subject prefix. Ben. --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -274,7 +274,7 @@ struct phy_device *of_phy_attach(struct } EXPORT_SYMBOL(of_phy_attach); -#if defined(CONFIG_FIXED_PHY) +#if IS_ENABLED(CONFIG_FIXED_PHY) /* * of_phy_is_fixed_link() and of_phy_register_fixed_link() must * support two DT bindings: --- a/include/linux/of_mdio.h +++ b/include/linux/of_mdio.h @@ -69,7 +69,7 @@ static inline int of_mdio_parse_addr(str } #endif /* CONFIG_OF */ -#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_FIXED_PHY) extern int of_phy_register_fixed_link(struct device_node *np); extern bool of_phy_is_fixed_link(struct device_node *np); #else -- Ben Hutchings Software Developer, Codethink Ltd.
[PATCH net-next] ti_cpsw: Check for disabled child nodes
Dual MAC devices don't necessarily have both MACs wired up, so ignore
those that are disabled.
Signed-off-by: Ben Hutchings
---
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -2023,7 +2023,7 @@ static int cpsw_probe_dt(struct cpsw_pri
if (ret)
dev_warn(&pdev->dev, "Doesn't have any child node\n");
- for_each_child_of_node(node, slave_node) {
+ for_each_available_child_of_node(node, slave_node) {
struct cpsw_slave_data *slave_data = data->slave_data + i;
const void *mac_addr = NULL;
int lenp;
--
Ben Hutchings
Software Developer, Codethink Ltd.
of_mdio: Enable fixed PHY support if driver is a module
The fixed_phy driver doesn't have to be built-in, and it's important that of_mdio supports it even if it's a module. Signed-off-by: Ben Hutchings --- --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -274,7 +274,7 @@ struct phy_device *of_phy_attach(struct } EXPORT_SYMBOL(of_phy_attach); -#if defined(CONFIG_FIXED_PHY) +#if IS_ENABLED(CONFIG_FIXED_PHY) /* * of_phy_is_fixed_link() and of_phy_register_fixed_link() must * support two DT bindings: --- a/include/linux/of_mdio.h +++ b/include/linux/of_mdio.h @@ -69,7 +69,7 @@ static inline int of_mdio_parse_addr(str } #endif /* CONFIG_OF */ -#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) +#if defined(CONFIG_OF) && IS_ENABLED(CONFIG_FIXED_PHY) extern int of_phy_register_fixed_link(struct device_node *np); extern bool of_phy_is_fixed_link(struct device_node *np); #else -- Ben Hutchings Software Developer, Codethink Ltd.
Re: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)
On Thu, 2016-06-16 at 10:55 -0700, Benjamin Poirier wrote:
> On 2016/06/13 11:46, Netanel Belgazal wrote:
[...]
> > +static ssize_t ena_show_small_copy_len(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct ena_adapter *adapter = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%d\n", adapter->small_copy_len);
> > +}
> > +
> > +static DEVICE_ATTR(small_copy_len, S_IRUGO | S_IWUSR,
> > ena_show_small_copy_len,
> > + ena_store_small_copy_len);
>
> This is what many other drivers call (rx_)copybreak. Perhaps it's time
> to add it to ethtool as well?
[...]
There is the 'tunable' ethtool API for random parameters like
rx_copybreak. The ethtool utility doesn't support it yet, though.
Ben.
--
Ben Hutchings
Life is what happens to you while you're busy making other plans.
- John
Lennon
signature.asc
Description: This is a digitally signed message part
Re: [PATCH net-next v4 5/7] vmxnet3: add support for get_coalesce, set_coalesce ethtool operations
On Tue, 2016-06-14 at 11:52 -0700, Shrikrishna Khare wrote:
> Signed-off-by: Keyong Sun
> Signed-off-by: Manoj Tammali
> Signed-off-by: Shrikrishna Khare
Reviewed-by: Ben Hutchings
> ---
> v1-v2: v1 patch used special values of rx-usecs to differentiate between
> coalescing modes. v2 uses relevant fields in struct ethtool_coalesce
> to choose modes. Also, a new command VMXNET3_CMD_GET_COALESCE is
> introduced which allows driver to query the device for default coalescing
> configuration.
>
> v3-v4: address Ben's review comments: remove unnecessary memset from
> vmxnet3_get_coalesce.
>
> ---
> drivers/net/vmxnet3/vmxnet3_defs.h| 33 ++-
> drivers/net/vmxnet3/vmxnet3_drv.c | 54
> drivers/net/vmxnet3/vmxnet3_ethtool.c | 158
> ++
> drivers/net/vmxnet3/vmxnet3_int.h | 9 ++
> 4 files changed, 253 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/vmxnet3/vmxnet3_defs.h
> b/drivers/net/vmxnet3/vmxnet3_defs.h
> index f3b31c2..274e145 100644
> --- a/drivers/net/vmxnet3/vmxnet3_defs.h
> +++ b/drivers/net/vmxnet3/vmxnet3_defs.h
> @@ -80,6 +80,7 @@ enum {
> VMXNET3_CMD_LOAD_PLUGIN,
> VMXNET3_CMD_RESERVED2,
> VMXNET3_CMD_RESERVED3,
> + VMXNET3_CMD_SET_COALESCE,
>
> VMXNET3_CMD_FIRST_GET = 0xF00D,
> VMXNET3_CMD_GET_QUEUE_STATUS = VMXNET3_CMD_FIRST_GET,
> @@ -92,7 +93,8 @@ enum {
> VMXNET3_CMD_GET_DEV_EXTRA_INFO,
> VMXNET3_CMD_GET_CONF_INTR,
> VMXNET3_CMD_GET_RESERVED1,
> - VMXNET3_CMD_GET_TXDATA_DESC_SIZE
> + VMXNET3_CMD_GET_TXDATA_DESC_SIZE,
> + VMXNET3_CMD_GET_COALESCE,
> };
>
> /*
> @@ -637,6 +639,35 @@ struct Vmxnet3_SetPolling {
> u8 enablePolling;
> };
>
> +#define VMXNET3_COAL_STATIC_MAX_DEPTH128
> +#define VMXNET3_COAL_RBC_MIN_RATE100
> +#define VMXNET3_COAL_RBC_MAX_RATE10
> +
> +enum Vmxnet3_CoalesceMode {
> + VMXNET3_COALESCE_DISABLED = 0,
> + VMXNET3_COALESCE_ADAPT = 1,
> + VMXNET3_COALESCE_STATIC = 2,
> + VMXNET3_COALESCE_RBC= 3
> +};
> +
> +struct Vmxnet3_CoalesceRbc {
> + u32 rbc_rate;
> +};
> +
> +struct Vmxnet3_CoalesceStatic {
> + u32 tx_depth;
> + u32 tx_comp_depth;
> + u32 rx_depth;
> +};
> +
> +struct Vmxnet3_CoalesceScheme {
> + enum Vmxnet3_CoalesceMode coalMode;
> + union {
> + struct Vmxnet3_CoalesceRbc coalRbc;
> + struct Vmxnet3_CoalesceStatic coalStatic;
> + } coalPara;
> +};
> +
> /* If the command data <= 16 bytes, use the shared memory directly.
> * otherwise, use variable length configuration descriptor.
> */
> diff --git a/drivers/net/vmxnet3/vmxnet3_drv.c
> b/drivers/net/vmxnet3/vmxnet3_drv.c
> index 6449d2e..d0bcc1d9 100644
> --- a/drivers/net/vmxnet3/vmxnet3_drv.c
> +++ b/drivers/net/vmxnet3/vmxnet3_drv.c
> @@ -2491,6 +2491,32 @@ vmxnet3_setup_driver_shared(struct vmxnet3_adapter
> *adapter)
> /* the rest are already zeroed */
> }
>
> +static void
> +vmxnet3_init_coalesce(struct vmxnet3_adapter *adapter)
> +{
> + struct Vmxnet3_DriverShared *shared = adapter->shared;
> + union Vmxnet3_CmdInfo *cmdInfo = &shared->cu.cmdInfo;
> + unsigned long flags;
> +
> + if (!VMXNET3_VERSION_GE_3(adapter))
> + return;
> +
> + spin_lock_irqsave(&adapter->cmd_lock, flags);
> + cmdInfo->varConf.confVer = 1;
> + cmdInfo->varConf.confLen =
> + cpu_to_le32(sizeof(*adapter->coal_conf));
> + cmdInfo->varConf.confPA = cpu_to_le64(adapter->coal_conf_pa);
> +
> + if (adapter->default_coal_mode) {
> + VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD,
> + VMXNET3_CMD_GET_COALESCE);
> + } else {
> + VMXNET3_WRITE_BAR1_REG(adapter, VMXNET3_REG_CMD,
> + VMXNET3_CMD_SET_COALESCE);
> + }
> +
> + spin_unlock_irqrestore(&adapter->cmd_lock, flags);
> +}
>
> int
> vmxnet3_activate_dev(struct vmxnet3_adapter *adapter)
> @@ -2540,6 +2566,8 @@ vmxnet3_activate_dev(struct vmxnet3_adapter *adapter)
> goto activate_err;
> }
>
> + vmxnet3_init_coalesce(adapter);
> +
> for (i = 0; i < adapter->num_rx_queues; i++) {
> VMXNET3_
Re: [PATCH net-next v3 5/7] vmxnet3: add support for get_coalesce, set_coalesce ethtool operations
On Mon, 2016-06-13 at 18:50 -0700, Shrikrishna Khare wrote:
[...]
> --- a/drivers/net/vmxnet3/vmxnet3_ethtool.c
> +++ b/drivers/net/vmxnet3/vmxnet3_ethtool.c
> @@ -725,6 +725,164 @@ vmxnet3_set_rss(struct net_device *netdev, const u32
> *p, const u8 *key,
> }
> #endif
>
> +static int
> +vmxnet3_get_coalesce(struct net_device *netdev, struct ethtool_coalesce *ec)
> +{
> + struct vmxnet3_adapter *adapter = netdev_priv(netdev);
> +
> + if (!VMXNET3_VERSION_GE_3(adapter))
> + return -EOPNOTSUPP;
> +
> + memset(ec, 0, sizeof(struct ethtool_coalesce));
[...]
The ethtool core already clears the structure, and it sets the cmd
field properly. This memset() should be removed.
Otherwise I think this is fine.
Ben.
--
Ben Hutchings
We get into the habit of living before acquiring the habit of thinking.
- Albert
Camus
signature.asc
Description: This is a digitally signed message part
Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes
On Sat, 2016-06-11 at 19:26 -0700, David Miller wrote: > From: Vidya Sagar Ravipati > Date: Sat, 11 Jun 2016 16:22:38 -0700 > > > As part of ethtool application, application is requesting the drivers > > to provide the supported eeprom size to allocate memory buffer for > > getting complete dump. > > And the right way to do that is the driver requests the eeprom info > with a buffer size of zero, then the driver fills in the size field > for what the size actually is. > > Then the application can allocate the proper buffer size and rerun > the eeprom request. > > Putting endless values for each and every eeprom type a device has is > just rediculous. > > I'm not going to continue promoting this broken and unscalable scheme, > we have to fix this. I don't think there's nothing broken here. ethtool doesn't use those macros, the drivers do. Ben. -- Ben Hutchings The program is absolutely right; therefore, the computer must be wrong. signature.asc Description: This is a digitally signed message part
Re: [PATCH net-next] ethtool: Macro definition for SFF-8436/8636 Memory map max sizes
On Sat, 2016-06-11 at 15:51 -0700, David Miller wrote: [...] > Why do we need these values in the header file at all? Because we don't like putting magic numbers in driver code, and these sizes are defined by standards that are independent of a single driver. > The application can probe the size by repeated eeprom calls, increasing > the buffer size each time as needed until success. But really it should use ETHTOOL_GMODULEINFO first. Ben. -- Ben Hutchings The program is absolutely right; therefore, the computer must be wrong. signature.asc Description: This is a digitally signed message part
Re: [RFC net-next 1/1] ethtool: Add support for set eeprom metadata.
On Sun, 2016-06-05 at 13:29 +, Yuval Mintz wrote:
> >
> > > > Currently ethtool implementation does not have a way to pass the
> > > > metadata for eeprom related operations. Some adapters have a
> > > > complicated non-volatile memory implementation that requires
> > > > additional information – there are drivers [bnx2x and bnxt] that use
> > > > the ‘magic’ field in the {G,S}EEPROM for that purpose, although that’s
> > > > not
> > its intended usage.
> > > >
> > > > This patch adds a provision to pass the eeprom metadata for
> > > > %ETHTOOL_SEEPROM/%ETHTOOL_GEEPROM implementations. User
> > provided
> > > > metadata will be cached by the driver and assigns a magic value
> > > > which the application need to use for the subsequent {G,S}EEPROM
> > command.
> > >
> > > Hi Dave,
> > >
> > > This got no comments at all.
> > > What do you want us to with it next? Should we re-send it as a patch?
> >
> > Here's a comment: I really dislike this.
> :-)
>
> > - It doesn't specify any semantics for the 'metadata'. The comment hints
> > that
> > they are driver-specific identifiers for different NVRAM partitions.
> Not exactly, but close [in our use case there are 2 'methods' of accessing the
> flash - either according to addresses or logical 'files'].
>
> > - It doesn't provide a way for userland to enumerate the valid metadata
> > values.
> I agree; I can't think of any good way of enumerating device-specific values.
>
> > - It's not clear whether the driver is supposed to maintain just one
> > metadata:magic mapping, or more than that.
> Theoretically, I guess it could maintain multiple, but that wasn't the
> intention.
> One should do.
>
> > Is the ethtool API really the right interface for access to flash? The sfc
> > driver
> > exposes a large number of flash partitions using MTD instead. These can be
> > enumerated (through /proc/mtd or sysfs) and they can be read and written
> > through block devices.
>
> I think the better question then is 'what's the purpose of this ethtool API
> at all'?
I think it's a bit of an accident - MTD was designed for flash in
embedded systems, and it used to have a static limit on the number of
partitions. The ethtool API was then rather better suited to plug-in
cards that would have a single small EEPROM.
> I agree we can go and do everything via MTD; The reason we've tried using this
> API was mainly... because it was there. And thus we thought this is the RIGHT
> method for providing users the way of reading their flash.
[...]
I think that MTD makes more sense for flash partitions, especially when
there are several of them. I already did the work of removing
the static limit on the number of partitions, and convincing
distributions to enable the MTD core drivers. (That said, you will
still find some users who need to change their custom kernel
configurations.)
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert
Einstein
signature.asc
Description: This is a digitally signed message part
Re: [RFC net-next 1/1] ethtool: Add support for set eeprom metadata.
On Sun, 2016-06-05 at 10:16 +, Yuval Mintz wrote:
> >
> > Currently ethtool implementation does not have a way to pass the metadata
> > for
> > eeprom related operations. Some adapters have a complicated non-volatile
> > memory implementation that requires additional information – there are
> > drivers
> > [bnx2x and bnxt] that use the ‘magic’ field in the {G,S}EEPROM for that
> > purpose,
> > although that’s not its intended usage.
> >
> > This patch adds a provision to pass the eeprom metadata for
> > %ETHTOOL_SEEPROM/%ETHTOOL_GEEPROM implementations. User provided
> > metadata will be cached by the driver and assigns a magic value which the
> > application need to use for the subsequent {G,S}EEPROM command.
>
> Hi Dave,
>
> This got no comments at all.
> What do you want us to with it next? Should we re-send it as a patch?
Here's a comment: I really dislike this.
- It doesn't specify any semantics for the 'metadata'. The comment
hints that they are driver-specific identifiers for different NVRAM
partitions.
- It doesn't provide a way for userland to enumerate the valid metadata
values.
- It's not clear whether the driver is supposed to maintain just one
metadata:magic mapping, or more than that.
Is the ethtool API really the right interface for access to flash? The
sfc driver exposes a large number of flash partitions using MTD
instead. These can be enumerated (through /proc/mtd or sysfs) and they
can be read and written through block devices.
Ben.
--
Ben Hutchings
Everything should be made as simple as possible, but not simpler.
- Albert
Einstein
signature.asc
Description: This is a digitally signed message part
Re: [PATCH net-next 2/2] bgmac: Add support for ethtool statistics
On Fri, 2016-06-03 at 10:07 -0700, Florian Fainelli wrote:
[...]
> +static void bgmac_get_strings(struct net_device *dev, u32 stringset,
> + u8 *data)
> +{
> + int i;
> +
> + if (stringset != ETH_SS_STATS)
> + return;
> +
> + for (i = 0; i < BGMAC_STATS_LEN; i++)
> + memcpy(data + i * ETH_GSTRING_LEN,
> + bgmac_get_strings_stats[i].name,
> + ETH_GSTRING_LEN);
These strings are null-terminated, not padded to ETH_GSTRING_LEN. So
here you should be using strlcpy() instead of memcpy().
> +}
> +
> +static void bgmac_get_ethtool_stats(struct net_device *dev,
> + struct ethtool_stats *ss, uint64_t *data)
> +{
> + struct bgmac *bgmac = netdev_priv(dev);
> + const struct bgmac_stat *s;
> + unsigned int i;
> + u64 val;
> +
> + if (!netif_running(dev))
> + return;
> +
> + for (i = 0; i < BGMAC_STATS_LEN; i++) {
> + s = &bgmac_get_strings_stats[i];
> + val = 0;
> + if (s->size == 8)
> + val = (u64)bgmac_read(bgmac, s->offset + 4);
Isn't this missing a << 32?
Does reading the high 32 bits latch the value of the low 32 bits? If
not, you need to read the high bits again after the low bits and retry
if they changed.
> + val |= bgmac_read(bgmac, s->offset);
> + data[i] = (u64)val;
Redundant cast.
Ben.
> + }
> +}
[...]
--
Ben Hutchings
Nothing is ever a complete failure; it can always serve as a bad
example.
signature.asc
Description: This is a digitally signed message part
Re: [PATCH] ethtool: fix a kernel infoleak in ethtool_get_pauseparam
On Wed, 2016-06-01 at 16:39 +0200, Kangjie Lu wrote:
> The field autoneg of pauseparam is not initialized in some
> implementations of get_pauseparam(),
Nonsense. The current implementation initialises all fields. (If
there was padding in the structure, this change would be needed to
guarantee that the padding was initialised. But there isn't.)
Ben.
> but the whole object is
> copied to userland.
>
> Signed-off-by: Kangjie Lu
> ---
> net/core/ethtool.c | 5 -
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index f426c5a..84544bd 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1723,7 +1723,10 @@ static noinline_for_stack int
> ethtool_set_channels(struct net_device *dev,
>
> static int ethtool_get_pauseparam(struct net_device *dev, void
> __user *useraddr)
> {
> - struct ethtool_pauseparam pauseparam = { ETHTOOL_GPAUSEPARAM
> };
> + struct ethtool_pauseparam pauseparam;
> +
> + memset(&pauseparam, 0, sizeof(pauseparam));
> + pauseparam.cmd = ETHTOOL_GPAUSEPARAM;
>
> if (!dev->ethtool_ops->get_pauseparam)
> return -EOPNOTSUPP;
--
Ben Hutchings
To err is human; to really foul things up requires a computer.
signature.asc
Description: This is a digitally signed message part
Re: [RESEND] Re: updating carl9170-1.fw in linux-firmware.git
On Wed, 2016-04-20 at 23:11 +0200, Christian Lamparter wrote: > On Wednesday, April 20, 2016 10:59:44 AM Kalle Valo wrote: > > Christian Lamparter writes: > > > > > On Monday, April 18, 2016 07:42:05 PM Kalle Valo wrote: > > > > Christian Lamparter writes: > > > > > > > > > On Monday, April 18, 2016 06:45:09 PM Kalle Valo wrote: > > > > > > > > > > > Why even mention anything about a "special firmware" as the > > > > > > firmware is > > > > > > already available from linux-firmware.git? > > > > > > > > > > Yes and no. 1.9.6 is in linux-firmware.git. I've tried to add 1.9.9 > > > > > too > > > > > but that failed. > > > > > > > > > > > > > Rick's comment makes sense to me, better just to provide the latest > > > > version. No need to unnecessary confuse the users. And if someone really > > > > wants to use an older version that she can retrieve it from the git > > > > history. > > > > > > Part of the fun here is that firmware is GPLv2. The linux-firmware.git has > > > to point to or add the firmware source to their tree. They have added > > > every > > > single source file to it instead of "packaging" it in a tar.bz2/gz/xz > > > like you normally do for release sources. > > > > > > If you want to read more about it: > > > <http://www.spinics.net/lists/linux-wireless/msg101868.html> > > > > Yeah, that's more work. I get that. But I'm still not understanding > > what's the actual problem which prevents us from updating carl9170 > > firmware in linux-firmware. > I'm not sure, but why not ask? I've added the cc'ed Linux Firmware > Maintainers. So for those people reading the fw list: > > What would it take to update the carl9170-1.fw firmware file in your > repository to the latest version? > > Who has to sent the firmware update. Does it have to be the person who > sent the first request? (Xose)? The maintainer of the firmware (me)? > someone from Qualcomm Atheros? Or someone else (specific)? (the > firmware is licensed as GPLv2 - in theory anyone should be able to > do that) Given the licence, I don't particularly care. > How should the firmware source update be handled? Currently the latest > .tar.xz of the firmware has ~130kb. The formated patches from 1.9.6 to > latest are about ~100kb (182 individual patches). Either patches that 'git am' can handle, or a git branch. > How does linux-firmware handle new binary firmware images and new > sources? What if carl9170fw-2.bin is added. Do we need another > source directory for this in the current tree then? Because > carl9170fw-1.bin will still be needed for backwards compatibility > so we basically need to duplicate parts of the source? We still need to include the old binary for compatibility, and the old source for GPL compliance. (If there was a source version that could build firmware for both ABI versions, then we could update both binaries and have one set of source files. But it doesn't sound like that's the case.) > Also, how's the situation with ath9k_htc? The 1.4.0 image contains > some GPLv2 code as well? I didn't realise that. > So, why is there no source in the tree, but just the link to it? An oversight which we need to fix. > Because, I would like to do basically the same > for carl9170fw and just add a link to the carl9170fw repository and > save everyone this source update "song and dance". Merely linking to upstream source doesn't satisfy GPLv2 source requirements, at least not in case of commercial distribution. Linux distributors should be able to use a snapshot of linux-firmware as the upstream source for a package, without worrying about whether there are extra sources they need to include. (I'm aware that there are several files that don't actually have clear licences for. But those are at least called out in WHENCE, and were previously distributed as part of the kernel sources for years.) Ben. -- Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once. signature.asc Description: This is a digitally signed message part
Re: [ethtool 0/3][pull request] Intel Wired LAN Driver Updates 2016-05-03
On Tue, 2016-05-24 at 16:47 -0700, Jeff Kirsher wrote: > On Wed, 2016-05-04 at 09:44 -0700, Jeff Kirsher wrote: > > This series contains updates to ixgbe in ethtool. > > > > Preethi adds missing device IDs and mac_type definitions, also updated > > the display registers for x550, x550em_x/a. Cleaned up the format string > > storage by taking advantage of "for" loops. > > > > The following are changes since commit > > deb1c6613ec14fd828d321e38c7bea45fe559bd5: > > Release version 4.5. > > and are available in the git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/jkirsher/ethtool master > > > > Preethi Banala (3): > > ethtool/ixgbe: Add device ID and mac_type definitions > > ethtool/ixgbe: Correct offsets and support x550, x550em_x, x550em_a > > ethtool/ixgbe: Reduce format string storage > > > > ixgbe.c | 173 +++--- > > -- > > 1 file changed, 95 insertions(+), 78 deletions(-) > > > > Is Ben still maintaining ethtool? I am - barely. > I ask because I have this series which I > sent out earlier this month, with no word and I know there is at least one > other ethtool patch series that has had no response or committal from who > ever is maintaining ethtool. I'm going to do one more release and then look for a new maintainer. Ben. > I know we discussed last netconf that we should look at possibly a new tool > to address the shortcomings of ethtool, but I was not aware we had > abandoned maintaining the current ethtool already before any replacement > tool has been developed. -- Ben Hutchings Time is nature's way of making sure that everything doesn't happen at once. signature.asc Description: This is a digitally signed message part
