* [PATCH bpf-next v2 2/3] bpf: btf: add btf print functionality
From: Okash Khawaja @ 2018-07-02 18:39 UTC (permalink / raw)
To: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, Jakub Kicinski, David S. Miller
Cc: netdev, kernel-team, linux-kernel
In-Reply-To: <20180702183913.669030439@fb.com>
[-- Attachment #1: 02-add-btf-dump-map.patch --]
[-- Type: text/plain, Size: 9608 bytes --]
This consumes functionality exported in the previous patch. It does the
main job of printing with BTF data. This is used in the following patch
to provide a more readable output of a map's dump. It relies on
json_writer to do json printing. Below is sample output where map keys
are ints and values are of type struct A:
typedef int int_type;
enum E {
E0,
E1,
};
struct B {
int x;
int y;
};
struct A {
int m;
unsigned long long n;
char o;
int p[8];
int q[4][8];
enum E r;
void *s;
struct B t;
const int u;
int_type v;
unsigned int w1: 3;
unsigned int w2: 3;
};
$ sudo bpftool map dump id 14
[{
"key": 0,
"value": {
"m": 1,
"n": 2,
"o": "c",
"p": [15,16,17,18,15,16,17,18
],
"q": [[25,26,27,28,25,26,27,28
],[35,36,37,38,35,36,37,38
],[45,46,47,48,45,46,47,48
],[55,56,57,58,55,56,57,58
]
],
"r": 1,
"s": 0x7ffd80531cf8,
"t": {
"x": 5,
"y": 10
},
"u": 100,
"v": 20,
"w1": 0x7,
"w2": 0x3
}
}
]
This patch uses json's {} and [] to imply struct/union and array. More
explicit information can be added later. For example, a command line
option can be introduced to print whether a key or value is struct
or union, name of a struct etc. This will however come at the expense
of duplicating info when, for example, printing an array of structs.
enums are printed as ints without their names.
Signed-off-by: Okash Khawaja <osk@fb.com>
---
tools/bpf/bpftool/btf_dumper.c | 263 +++++++++++++++++++++++++++++++++++++++++
tools/bpf/bpftool/btf_dumper.h | 23 +++
2 files changed, 286 insertions(+)
--- /dev/null
+++ b/tools/bpf/bpftool/btf_dumper.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018 Facebook */
+
+#include <linux/btf.h>
+#include <linux/err.h>
+#include <stdio.h> /* for (FILE *) used by json_writer */
+#include <linux/bitops.h>
+#include <string.h>
+#include <ctype.h>
+
+#include "btf.h"
+#include "json_writer.h"
+#include "btf_dumper.h"
+
+#define BITS_PER_BYTE_MASK (BITS_PER_BYTE - 1)
+#define BITS_PER_BYTE_MASKED(bits) ((bits) & BITS_PER_BYTE_MASK)
+#define BITS_ROUNDDOWN_BYTES(bits) ((bits) >> 3)
+#define BITS_ROUNDUP_BYTES(bits) \
+ (BITS_ROUNDDOWN_BYTES(bits) + !!BITS_PER_BYTE_MASKED(bits))
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+ uint8_t bit_offset, const void *data);
+
+static void btf_dumper_ptr(const void *data, json_writer_t *jw,
+ bool is_plain_text)
+{
+ if (is_plain_text)
+ jsonw_printf(jw, "%p", *((uintptr_t *)data));
+ else
+ jsonw_printf(jw, "%u", *((uintptr_t *)data));
+}
+
+static int btf_dumper_modifier(const struct btf_dumper *d, uint32_t type_id,
+ const void *data)
+{
+ int32_t actual_type_id = btf__resolve_type(d->btf, type_id);
+ int ret;
+
+ if (actual_type_id < 0)
+ return actual_type_id;
+
+ ret = btf_dumper_do_type(d, actual_type_id, 0, data);
+
+ return ret;
+}
+
+static void btf_dumper_enum(const void *data, json_writer_t *jw)
+{
+ jsonw_printf(jw, "%d", *((int32_t *)data));
+}
+
+static int btf_dumper_array(const struct btf_dumper *d, uint32_t type_id,
+ const void *data)
+{
+ const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+ struct btf_array *arr = (struct btf_array *)(t + 1);
+ int64_t elem_size;
+ int ret = 0;
+ uint32_t i;
+
+ elem_size = btf__resolve_size(d->btf, arr->type);
+ if (elem_size < 0)
+ return elem_size;
+
+ jsonw_start_array(d->jw);
+ for (i = 0; i < arr->nelems; i++) {
+ ret = btf_dumper_do_type(d, arr->type, 0,
+ data + (i * elem_size));
+ if (ret)
+ break;
+ }
+
+ jsonw_end_array(d->jw);
+ return ret;
+}
+
+static void btf_dumper_int_bits(uint32_t int_type, uint8_t bit_offset,
+ const void *data, json_writer_t *jw,
+ bool is_plain_text)
+{
+ uint32_t bits = BTF_INT_BITS(int_type);
+ uint16_t total_bits_offset;
+ uint16_t bytes_to_copy;
+ uint16_t bits_to_copy;
+ uint8_t upper_bits;
+ union {
+ uint64_t u64_num;
+ uint8_t u8_nums[8];
+ } print_num;
+
+ total_bits_offset = bit_offset + BTF_INT_OFFSET(int_type);
+ data += BITS_ROUNDDOWN_BYTES(total_bits_offset);
+ bit_offset = BITS_PER_BYTE_MASKED(total_bits_offset);
+ bits_to_copy = bits + bit_offset;
+ bytes_to_copy = BITS_ROUNDUP_BYTES(bits_to_copy);
+
+ print_num.u64_num = 0;
+ memcpy(&print_num.u64_num, data, bytes_to_copy);
+
+ upper_bits = BITS_PER_BYTE_MASKED(bits_to_copy);
+ if (upper_bits) {
+ uint8_t mask = (1 << upper_bits) - 1;
+
+ print_num.u8_nums[bytes_to_copy - 1] &= mask;
+ }
+
+ print_num.u64_num >>= bit_offset;
+
+ if (is_plain_text)
+ jsonw_printf(jw, "0x%llx", print_num.u64_num);
+ else
+ jsonw_printf(jw, "%llu", print_num.u64_num);
+}
+
+static int btf_dumper_int(const struct btf_type *t, uint8_t bit_offset,
+ const void *data, json_writer_t *jw,
+ bool is_plain_text)
+{
+ uint32_t *int_type = (uint32_t *)(t + 1);
+ uint32_t bits = BTF_INT_BITS(*int_type);
+ int ret = 0;
+
+ /* if this is bit field */
+ if (bit_offset || BTF_INT_OFFSET(*int_type) ||
+ BITS_PER_BYTE_MASKED(bits)) {
+ btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+ is_plain_text);
+ return ret;
+ }
+
+ switch (BTF_INT_ENCODING(*int_type)) {
+ case 0:
+ if (BTF_INT_BITS(*int_type) == 64)
+ jsonw_printf(jw, "%lu", *((uint64_t *)data));
+ else if (BTF_INT_BITS(*int_type) == 32)
+ jsonw_printf(jw, "%u", *((uint32_t *)data));
+ else if (BTF_INT_BITS(*int_type) == 16)
+ jsonw_printf(jw, "%hu", *((uint16_t *)data));
+ else if (BTF_INT_BITS(*int_type) == 8)
+ jsonw_printf(jw, "%hhu", *((uint8_t *)data));
+ else
+ btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+ is_plain_text);
+ break;
+ case BTF_INT_SIGNED:
+ if (BTF_INT_BITS(*int_type) == 64)
+ jsonw_printf(jw, "%ld", *((int64_t *)data));
+ else if (BTF_INT_BITS(*int_type) == 32)
+ jsonw_printf(jw, "%d", *((int32_t *)data));
+ else if (BTF_INT_BITS(*int_type) == 16)
+ jsonw_printf(jw, "%hd", *((int16_t *)data));
+ else if (BTF_INT_BITS(*int_type) == 8)
+ jsonw_printf(jw, "%hhd", *((int8_t *)data));
+ else
+ btf_dumper_int_bits(*int_type, bit_offset, data, jw,
+ is_plain_text);
+ break;
+ case BTF_INT_CHAR:
+ if (*((char *)data) == '\0')
+ jsonw_null(jw);
+ else if (isprint(*((char *)data)))
+ jsonw_printf(jw, "\"%c\"", *((char *)data));
+ else
+ if (is_plain_text)
+ jsonw_printf(jw, "%hhx", *((char *)data));
+ else
+ jsonw_printf(jw, "%hhd", *((char *)data));
+ break;
+ case BTF_INT_BOOL:
+ jsonw_bool(jw, *((int *)data));
+ break;
+ default:
+ /* shouldn't happen */
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+static int btf_dumper_struct(const struct btf_dumper *d, uint32_t type_id,
+ const void *data)
+{
+ const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+ struct btf_member *m;
+ int ret = 0;
+ int i, vlen;
+
+ if (!t)
+ return -EINVAL;
+
+ vlen = BTF_INFO_VLEN(t->info);
+ jsonw_start_object(d->jw);
+ m = (struct btf_member *)(t + 1);
+
+ for (i = 0; i < vlen; i++) {
+ jsonw_name(d->jw, btf__name_by_offset(d->btf, m[i].name_off));
+ ret = btf_dumper_do_type(d, m[i].type,
+ BITS_PER_BYTE_MASKED(m[i].offset), data
+ + BITS_ROUNDDOWN_BYTES(m[i].offset));
+ if (ret)
+ return ret;
+ }
+
+ jsonw_end_object(d->jw);
+
+ return 0;
+}
+
+static int btf_dumper_do_type(const struct btf_dumper *d, uint32_t type_id,
+ uint8_t bit_offset, const void *data)
+{
+ const struct btf_type *t = btf__type_by_id(d->btf, type_id);
+ int ret = 0;
+
+ switch (BTF_INFO_KIND(t->info)) {
+ case BTF_KIND_INT:
+ ret = btf_dumper_int(t, bit_offset, data, d->jw,
+ d->is_plain_text);
+ break;
+ case BTF_KIND_STRUCT:
+ case BTF_KIND_UNION:
+ ret = btf_dumper_struct(d, type_id, data);
+ break;
+ case BTF_KIND_ARRAY:
+ ret = btf_dumper_array(d, type_id, data);
+ break;
+ case BTF_KIND_ENUM:
+ btf_dumper_enum(data, d->jw);
+ break;
+ case BTF_KIND_PTR:
+ btf_dumper_ptr(data, d->jw, d->is_plain_text);
+ break;
+ case BTF_KIND_UNKN:
+ jsonw_printf(d->jw, "(unknown)");
+ break;
+ case BTF_KIND_FWD:
+ /* map key or value can't be forward */
+ ret = -EINVAL;
+ break;
+ case BTF_KIND_TYPEDEF:
+ case BTF_KIND_VOLATILE:
+ case BTF_KIND_CONST:
+ case BTF_KIND_RESTRICT:
+ ret = btf_dumper_modifier(d, type_id, data);
+ break;
+ default:
+ jsonw_printf(d->jw, "(unsupported-kind");
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+ const void *data)
+{
+ if (!d)
+ return -EINVAL;
+
+ return btf_dumper_do_type(d, type_id, 0, data);
+}
--- /dev/null
+++ b/tools/bpf/bpftool/btf_dumper.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018 Facebook */
+
+#ifndef BTF_DUMPER_H
+#define BTF_DUMPER_H
+
+struct btf_dumper {
+ const struct btf *btf;
+ json_writer_t *jw;
+ bool is_plain_text;
+};
+
+/* btf_dumper_type - print data along with type information
+ * @d: an instance containing context for dumping types
+ * @type_id: index in btf->types array. this points to the type to be dumped
+ * @data: pointer the actual data, i.e. the values to be printed
+ *
+ * Returns zero on success and negative error code otherwise
+ */
+int32_t btf_dumper_type(const struct btf_dumper *d, uint32_t type_id,
+ const void *data);
+
+#endif
^ permalink raw reply
* [PATCH bpf-next v2 3/3] bpf: btf: print map dump and lookup with btf info
From: Okash Khawaja @ 2018-07-02 18:39 UTC (permalink / raw)
To: Daniel Borkmann, Martin KaFai Lau, Alexei Starovoitov,
Yonghong Song, Quentin Monnet, Jakub Kicinski, David S. Miller
Cc: netdev, kernel-team, linux-kernel
In-Reply-To: <20180702183913.669030439@fb.com>
[-- Attachment #1: 03-json-print-btf-info-for-map --]
[-- Type: text/plain, Size: 7734 bytes --]
This patch augments the output of bpftool's map dump and map lookup
commands to print data along side btf info, if the correspondin btf
info is available. The outputs for each of map dump and map lookup
commands are augmented in two ways:
1. when neither of -j and -p are supplied, btf-ful map data is printed
whose aim is human readability. This means no commitments for json- or
backward- compatibility.
2. when either -j or -p are supplied, a new json object named
"formatted" is added for each key-value pair. This object contains the
same data as the key-value pair, but with btf info. "formatted" object
promises json- and backward- compatibility. Below is a sample output.
$ bpftool map dump -p id 8
[{
"key": ["0x0f","0x00","0x00","0x00"
],
"value": ["0x03", "0x00", "0x00", "0x00", ...
],
"formatted": {
"key": 15,
"value": {
"int_field": 3,
...
}
}
}
]
This patch calls btf_dumper introduced in previous patch to accomplish
the above. Indeed, btf-ful info is only displayed if btf data for the
given map is available. Otherwise existing output is displayed as-is.
Signed-off-by: Okash Khawaja <osk@fb.com>
---
tools/bpf/bpftool/map.c | 174 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 166 insertions(+), 8 deletions(-)
--- a/tools/bpf/bpftool/map.c
+++ b/tools/bpf/bpftool/map.c
@@ -41,9 +41,13 @@
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
+#include <linux/err.h>
#include <bpf.h>
+#include "json_writer.h"
+#include "btf.h"
+#include "btf_dumper.h"
#include "main.h"
static const char * const map_type_name[] = {
@@ -148,8 +152,99 @@ int map_parse_fd_and_info(int *argc, cha
return fd;
}
+static int do_dump_btf(const struct btf_dumper *d,
+ struct bpf_map_info *map_info, void *key,
+ void *value)
+{
+ int ret;
+
+ /* start of key-value pair */
+ jsonw_start_object(d->jw);
+
+ jsonw_name(d->jw, "key");
+
+ ret = btf_dumper_type(d, map_info->btf_key_type_id, key);
+ if (ret)
+ return ret;
+
+ jsonw_name(d->jw, "value");
+
+ ret = btf_dumper_type(d, map_info->btf_value_type_id, value);
+
+ /* end of key-value pair */
+ jsonw_end_object(d->jw);
+
+ return ret;
+}
+
+static struct btf *get_btf(struct bpf_map_info *map_info)
+{
+ int btf_fd = bpf_btf_get_fd_by_id(map_info->btf_id);
+ struct bpf_btf_info btf_info = { 0 };
+ __u32 len = sizeof(btf_info);
+ void *ptr = NULL, *temp_ptr;
+ struct btf *btf = NULL;
+ uint32_t last_size;
+ int err;
+
+ if (btf_fd < 0)
+ return NULL;
+
+ /* we won't know btf_size until we call bpf_obj_get_info_by_fd(). so
+ * let's start with a sane default - 4KiB here - and resize it only if
+ * bpf_obj_get_info_by_fd() needs a bigger buffer. the do-while loop
+ * below should run a maximum of two iterations and that will be when
+ * we have to resize to a bigger buffer.
+ */
+ btf_info.btf_size = 4096;
+ do {
+ last_size = btf_info.btf_size;
+ temp_ptr = realloc(ptr, last_size);
+ if (!temp_ptr) {
+ p_err("unable to allocate memory for debug info");
+ goto exit_free;
+ }
+
+ ptr = temp_ptr;
+ bzero(ptr, last_size);
+ btf_info.btf = ptr_to_u64(ptr);
+ err = bpf_obj_get_info_by_fd(btf_fd, &btf_info, &len);
+ } while (!err && btf_info.btf_size > last_size && last_size == 4096);
+
+ if (err || btf_info.btf_size > last_size) {
+ p_info("can't get btf info. debug info won't be displayed. error: %s",
+ err ? strerror(errno) : "exceeds size retry");
+ goto exit_free;
+ }
+
+ btf = btf__new((uint8_t *)btf_info.btf,
+ btf_info.btf_size, NULL);
+ if (IS_ERR(btf)) {
+ printf("error when initialising btf: %s\n",
+ strerror(PTR_ERR(btf)));
+ btf = NULL;
+ }
+
+exit_free:
+ close(btf_fd);
+ free(ptr);
+
+ return btf;
+}
+
+static json_writer_t *get_btf_writer(void)
+{
+ json_writer_t *jw = jsonw_new(stdout);
+
+ if (!jw)
+ return NULL;
+ jsonw_pretty(jw, true);
+
+ return jw;
+}
+
static void print_entry_json(struct bpf_map_info *info, unsigned char *key,
- unsigned char *value)
+ unsigned char *value, struct btf *btf)
{
jsonw_start_object(json_wtr);
@@ -158,6 +253,15 @@ static void print_entry_json(struct bpf_
print_hex_data_json(key, info->key_size);
jsonw_name(json_wtr, "value");
print_hex_data_json(value, info->value_size);
+ if (btf) {
+ struct btf_dumper d = {
+ .btf = btf,
+ .jw = json_wtr,
+ .is_plain_text = false,
+ };
+ jsonw_name(json_wtr, "formatted");
+ do_dump_btf(&d, info, key, value);
+ }
} else {
unsigned int i, n;
@@ -508,10 +612,12 @@ static int do_show(int argc, char **argv
static int do_dump(int argc, char **argv)
{
+ struct bpf_map_info info = {};
void *key, *value, *prev_key;
unsigned int num_elems = 0;
- struct bpf_map_info info = {};
__u32 len = sizeof(info);
+ json_writer_t *btf_wtr;
+ struct btf *btf = NULL;
int err;
int fd;
@@ -537,8 +643,22 @@ static int do_dump(int argc, char **argv
}
prev_key = NULL;
+
+ btf = get_btf(&info);
if (json_output)
jsonw_start_array(json_wtr);
+ else
+ if (btf) {
+ btf_wtr = get_btf_writer();
+ if (!btf_wtr) {
+ p_info("failed to create json writer for btf. falling back to plain output");
+ btf__free(btf);
+ btf = NULL;
+ } else {
+ jsonw_start_array(btf_wtr);
+ }
+ }
+
while (true) {
err = bpf_map_get_next_key(fd, prev_key, key);
if (err) {
@@ -549,9 +669,18 @@ static int do_dump(int argc, char **argv
if (!bpf_map_lookup_elem(fd, key, value)) {
if (json_output)
- print_entry_json(&info, key, value);
+ print_entry_json(&info, key, value, btf);
else
- print_entry_plain(&info, key, value);
+ if (btf) {
+ struct btf_dumper d = {
+ .btf = btf,
+ .jw = btf_wtr,
+ .is_plain_text = true,
+ };
+ do_dump_btf(&d, &info, key, value);
+ } else {
+ print_entry_plain(&info, key, value);
+ }
} else {
if (json_output) {
jsonw_name(json_wtr, "key");
@@ -574,14 +703,19 @@ static int do_dump(int argc, char **argv
if (json_output)
jsonw_end_array(json_wtr);
- else
+ else if (btf) {
+ jsonw_end_array(btf_wtr);
+ jsonw_destroy(&btf_wtr);
+ } else {
printf("Found %u element%s\n", num_elems,
num_elems != 1 ? "s" : "");
+ }
exit_free:
free(key);
free(value);
close(fd);
+ btf__free(btf);
return err;
}
@@ -637,6 +771,8 @@ static int do_lookup(int argc, char **ar
{
struct bpf_map_info info = {};
__u32 len = sizeof(info);
+ json_writer_t *btf_wtr;
+ struct btf *btf = NULL;
void *key, *value;
int err;
int fd;
@@ -662,10 +798,31 @@ static int do_lookup(int argc, char **ar
err = bpf_map_lookup_elem(fd, key, value);
if (!err) {
- if (json_output)
- print_entry_json(&info, key, value);
- else
+ btf = get_btf(&info);
+ if (json_output) {
+ print_entry_json(&info, key, value, btf);
+ } else if (btf) {
+ /* if here json_wtr wouldn't have been initialised,
+ * so let's create separate writer for btf
+ */
+ btf_wtr = get_btf_writer();
+ if (!btf_wtr) {
+ p_info("failed to create json writer for btf. falling back to plain output");
+ btf__free(btf);
+ btf = NULL;
+ print_entry_plain(&info, key, value);
+ } else {
+ struct btf_dumper d = {
+ .btf = btf,
+ .jw = btf_wtr,
+ .is_plain_text = true,
+ };
+ do_dump_btf(&d, &info, key, value);
+ jsonw_destroy(&btf_wtr);
+ }
+ } else {
print_entry_plain(&info, key, value);
+ }
} else if (errno == ENOENT) {
if (json_output) {
jsonw_null(json_wtr);
@@ -682,6 +839,7 @@ exit_free:
free(key);
free(value);
close(fd);
+ btf__free(btf);
return err;
}
^ permalink raw reply
* Re: [PATCH bpf 3/3] bpf: undo prog rejection on read-only lock failure
From: Kees Cook @ 2018-07-02 18:48 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: Alexei Starovoitov, Network Development, Laura Abbott
In-Reply-To: <564389fa-0233-e1eb-9b0a-f2ffa30104da@iogearbox.net>
On Fri, Jun 29, 2018 at 4:47 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 06/29/2018 08:42 PM, Kees Cook wrote:
>> On Thu, Jun 28, 2018 at 2:34 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>>> Kees suggested that if set_memory_*() can fail, we should annotate it with
>>> __must_check, and all callers need to deal with it gracefully given those
>>> set_memory_*() markings aren't "advisory", but they're expected to actually
>>> do what they say. This might be an option worth to move forward in future
>>> but would at the same time require that set_memory_*() calls from supporting
>>> archs are guaranteed to be "atomic" in that they provide rollback if part
>>> of the range fails, once that happened, the transition from RW -> RO could
>>> be made more robust that way, while subsequent RO -> RW transition /must/
>>> continue guaranteeing to always succeed the undo part.
>>
>> Does this mean we can have BPF filters that aren't read-only then?
>> What's the situation where set_memory_ro() fails? (Can it be induced
>> by the user?)
>
> My understanding is that the cpa_process_alias() would attempt to also change
> attributes of physmap ranges, and it found that a large page had to be split
> for this but failed in doing so thus attributes couldn't be updated there due
> to page alloc error. Attempting to change the primary mapping which would be
> directly the addr passed to set_memory_ro() was however set to read-only
> despite error. While for reproduction I had a toggle on the alloc_pages() in
> split_large_page() to have it fail, I only could trigger it occasionally; I
> used the selftest suite in a loop to stress test and it hit about or twice
> over hours.
Okay, so it's pretty rare; that's good! :P
It really seems like this should be a situation that never fails, but
if we ARE going to allow failures, then I think we need to propagate
them up to callers. That means modules could fail to load in these
cases, etc, etc. Since this is a fundamental protection, we need to
either never fail to set things RO or we need to disallow operation
continuing in the face of something NOT being RO.
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply
* Re: WARNING: ODEBUG bug in sock_hash_free
From: John Fastabend @ 2018-07-02 18:48 UTC (permalink / raw)
To: syzbot, ast, daniel, linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <00000000000037c7d5056f84cabc@google.com>
On 06/25/2018 10:30 PM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: f0dc7f9c6dd9 Merge git://git.kernel.org/pub/scm/linux/kern..
> git tree: bpf-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=1725589f800000
> kernel config: https://syzkaller.appspot.com/x/.config?x=fa9c20c48788d1c1
> dashboard link: https://syzkaller.appspot.com/bug?extid=71aeaaf993d216185076
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+71aeaaf993d216185076@syzkaller.appspotmail.com
>
> ------------[ cut here ]------------
> ODEBUG: free active (active state 1) object type: rcu_head hint: (null)
> WARNING: CPU: 1 PID: 4959 at lib/debugobjects.c:329 debug_print_object+0x16a/0x210 lib/debugobjects.c:326
> Kernel panic - not syncing: panic_on_warn set ...
>
> CPU: 1 PID: 4959 Comm: kworker/1:3 Not tainted 4.17.0+ #39
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: events bpf_map_free_deferred
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x1b9/0x294 lib/dump_stack.c:113
> panic+0x22f/0x4de kernel/panic.c:184
> __warn.cold.8+0x163/0x1b3 kernel/panic.c:536
> report_bug+0x252/0x2d0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:178 [inline]
> do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
> do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
> invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:992
> RIP: 0010:debug_print_object+0x16a/0x210 lib/debugobjects.c:326
> Code: 1a 88 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85 92 00 00 00 48 8b 14 dd 60 75 1a 88 4c 89 f6 48 c7 c7 e0 6a 1a 88 e8 06 62 ec fd <0f> 0b 83 05 39 5b 44 06 01 48 83 c4 18 5b 41 5c 41 5d 41 5e 41 5f
> RSP: 0018:ffff880198e47490 EFLAGS: 00010082
> RAX: 0000000000000051 RBX: 0000000000000003 RCX: ffffffff81854ed8
> RDX: 0000000000000000 RSI: ffffffff8161f371 RDI: 0000000000000001
> RBP: ffff880198e474d0 R08: ffff8801d84b2240 R09: ffffed003b5e3ec2
> R10: ffffed003b5e3ec2 R11: ffff8801daf1f617 R12: 0000000000000001
> R13: ffffffff88f91d80 R14: ffffffff881a6f80 R15: 0000000000000000
> __debug_check_no_obj_freed lib/debugobjects.c:783 [inline]
> debug_check_no_obj_freed+0x3a6/0x584 lib/debugobjects.c:815
> kfree+0xc7/0x260 mm/slab.c:3812
> sock_hash_free+0x24e/0x6e0 kernel/bpf/sockmap.c:2093
> bpf_map_free_deferred+0xba/0xf0 kernel/bpf/syscall.c:262
> process_one_work+0xc64/0x1b70 kernel/workqueue.c:2153
> worker_thread+0x181/0x13a0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:240
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.17.0+ #39 Not tainted
> ------------------------------------------------------
> kworker/1:3/4959 is trying to acquire lock:
> 00000000190110fa ((console_sem).lock){-...}, at: down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
>
> but task is already holding lock:
> 00000000af3150e8 (&obj_hash[i].lock){-.-.}, at: __debug_check_no_obj_freed lib/debugobjects.c:774 [inline]
> 00000000af3150e8 (&obj_hash[i].lock){-.-.}, at: debug_check_no_obj_freed+0x159/0x584 lib/debugobjects.c:815
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #3 (&obj_hash[i].lock){-.-.}:
> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
> __debug_object_init+0x11f/0x12c0 lib/debugobjects.c:381
> debug_object_init+0x16/0x20 lib/debugobjects.c:429
> debug_hrtimer_init kernel/time/hrtimer.c:410 [inline]
> debug_init kernel/time/hrtimer.c:458 [inline]
> hrtimer_init+0x8f/0x460 kernel/time/hrtimer.c:1308
> init_dl_task_timer+0x1b/0x50 kernel/sched/deadline.c:1056
> __sched_fork+0x2a8/0x570 kernel/sched/core.c:2184
> init_idle+0x75/0x7a0 kernel/sched/core.c:5404
> sched_init+0xbeb/0xd10 kernel/sched/core.c:6102
> start_kernel+0x475/0x92d init/main.c:602
> x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
> x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
> secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
>
> -> #2 (&rq->lock){-.-.}:
> __raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
> _raw_spin_lock+0x2a/0x40 kernel/locking/spinlock.c:144
> rq_lock kernel/sched/sched.h:1805 [inline]
> task_fork_fair+0x8a/0x660 kernel/sched/fair.c:9953
> sched_fork+0x43e/0xb30 kernel/sched/core.c:2380
> copy_process.part.38+0x1bf1/0x7180 kernel/fork.c:1765
> copy_process kernel/fork.c:1608 [inline]
> _do_fork+0x291/0x12a0 kernel/fork.c:2091
> kernel_thread+0x34/0x40 kernel/fork.c:2150
> rest_init+0x22/0xe4 init/main.c:408
> start_kernel+0x906/0x92d init/main.c:738
> x86_64_start_reservations+0x29/0x2b arch/x86/kernel/head64.c:452
> x86_64_start_kernel+0x76/0x79 arch/x86/kernel/head64.c:433
> secondary_startup_64+0xa5/0xb0 arch/x86/kernel/head_64.S:242
>
> -> #1 (&p->pi_lock){-.-.}:
> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
> try_to_wake_up+0xca/0x1280 kernel/sched/core.c:1984
> wake_up_process+0x10/0x20 kernel/sched/core.c:2147
> __up.isra.1+0x1b8/0x290 kernel/locking/semaphore.c:262
> up+0x12f/0x1b0 kernel/locking/semaphore.c:187
> __up_console_sem+0xbe/0x1b0 kernel/printk/printk.c:242
> console_unlock+0x79a/0x10a0 kernel/printk/printk.c:2411
> vprintk_emit+0x6b2/0xde0 kernel/printk/printk.c:1907
> vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
> vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
> printk+0x9e/0xba kernel/printk/printk.c:1981
> load_umh+0x51/0xbd net/bpfilter/bpfilter_kern.c:99
> do_one_initcall+0x127/0x913 init/main.c:884
> do_initcall_level init/main.c:952 [inline]
> do_initcalls init/main.c:960 [inline]
> do_basic_setup init/main.c:978 [inline]
> kernel_init_freeable+0x49b/0x58e init/main.c:1135
> kernel_init+0x11/0x1b3 init/main.c:1061
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> -> #0 ((console_sem).lock){-...}:
> lock_acquire+0x1dc/0x520 kernel/locking/lockdep.c:3924
> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
> down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
> __down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
> console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
> console_trylock_spinning kernel/printk/printk.c:1643 [inline]
> vprintk_emit+0x699/0xde0 kernel/printk/printk.c:1906
> vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
> vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
> printk+0x9e/0xba kernel/printk/printk.c:1981
> __warn_printk+0x83/0xd0 kernel/panic.c:590
> debug_print_object+0x16a/0x210 lib/debugobjects.c:326
> __debug_check_no_obj_freed lib/debugobjects.c:783 [inline]
> debug_check_no_obj_freed+0x3a6/0x584 lib/debugobjects.c:815
> kfree+0xc7/0x260 mm/slab.c:3812
> sock_hash_free+0x24e/0x6e0 kernel/bpf/sockmap.c:2093
> bpf_map_free_deferred+0xba/0xf0 kernel/bpf/syscall.c:262
> process_one_work+0xc64/0x1b70 kernel/workqueue.c:2153
> worker_thread+0x181/0x13a0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:240
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
>
> other info that might help us debug this:
>
> Chain exists of:
> (console_sem).lock --> &rq->lock --> &obj_hash[i].lock
>
> Possible unsafe locking scenario:
>
> CPU0 CPU1
> ---- ----
> lock(&obj_hash[i].lock);
> lock(&rq->lock);
> lock(&obj_hash[i].lock);
> lock((console_sem).lock);
>
> *** DEADLOCK ***
>
> 4 locks held by kworker/1:3/4959:
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: __write_once_size include/linux/compiler.h:215 [inline]
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: arch_atomic64_set arch/x86/include/asm/atomic64_64.h:34 [inline]
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: atomic64_set include/asm-generic/atomic-instrumented.h:40 [inline]
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: atomic_long_set include/asm-generic/atomic-long.h:59 [inline]
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: set_work_data kernel/workqueue.c:617 [inline]
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: set_work_pool_and_clear_pending kernel/workqueue.c:644 [inline]
> #0: 00000000f67deee4 ((wq_completion)"events"){+.+.}, at: process_one_work+0xb35/0x1b70 kernel/workqueue.c:2124
> #1: 00000000776b40d0 ((work_completion)(&map->work)){+.+.}, at: process_one_work+0xb8c/0x1b70 kernel/workqueue.c:2128
> #2: 000000002a359661 (rcu_read_lock){....}, at: sock_hash_free+0x0/0x6e0 include/net/sock.h:2176
> #3: 00000000af3150e8 (&obj_hash[i].lock){-.-.}, at: __debug_check_no_obj_freed lib/debugobjects.c:774 [inline]
> #3: 00000000af3150e8 (&obj_hash[i].lock){-.-.}, at: debug_check_no_obj_freed+0x159/0x584 lib/debugobjects.c:815
>
> stack backtrace:
> CPU: 1 PID: 4959 Comm: kworker/1:3 Not tainted 4.17.0+ #39
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Workqueue: events bpf_map_free_deferred
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x1b9/0x294 lib/dump_stack.c:113
> print_circular_bug.isra.36.cold.56+0x1bd/0x27d kernel/locking/lockdep.c:1227
> check_prev_add kernel/locking/lockdep.c:1867 [inline]
> check_prevs_add kernel/locking/lockdep.c:1980 [inline]
> validate_chain kernel/locking/lockdep.c:2421 [inline]
> __lock_acquire+0x343e/0x5140 kernel/locking/lockdep.c:3435
> lock_acquire+0x1dc/0x520 kernel/locking/lockdep.c:3924
> __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline]
> _raw_spin_lock_irqsave+0x96/0xc0 kernel/locking/spinlock.c:152
> down_trylock+0x13/0x70 kernel/locking/semaphore.c:136
> __down_trylock_console_sem+0xae/0x200 kernel/printk/printk.c:225
> console_trylock+0x15/0xa0 kernel/printk/printk.c:2230
> console_trylock_spinning kernel/printk/printk.c:1643 [inline]
> vprintk_emit+0x699/0xde0 kernel/printk/printk.c:1906
> vprintk_default+0x28/0x30 kernel/printk/printk.c:1948
> vprintk_func+0x7a/0xe7 kernel/printk/printk_safe.c:382
> printk+0x9e/0xba kernel/printk/printk.c:1981
> __warn_printk+0x83/0xd0 kernel/panic.c:590
> debug_print_object+0x16a/0x210 lib/debugobjects.c:326
> __debug_check_no_obj_freed lib/debugobjects.c:783 [inline]
> debug_check_no_obj_freed+0x3a6/0x584 lib/debugobjects.c:815
> kfree+0xc7/0x260 mm/slab.c:3812
> sock_hash_free+0x24e/0x6e0 kernel/bpf/sockmap.c:2093
> bpf_map_free_deferred+0xba/0xf0 kernel/bpf/syscall.c:262
> process_one_work+0xc64/0x1b70 kernel/workqueue.c:2153
> worker_thread+0x181/0x13a0 kernel/workqueue.c:2296
> kthread+0x345/0x410 kernel/kthread.c:240
> ret_from_fork+0x3a/0x50 arch/x86/entry/entry_64.S:412
> Shutting down cpus with NMI
> Dumping ftrace buffer:
> (ftrace buffer empty)
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with syzbot.
#syz fix: bpf: sockhash fix omitted bucket lock in sock_close
^ permalink raw reply
* Re: possible deadlock in sock_hash_free
From: John Fastabend @ 2018-07-02 18:50 UTC (permalink / raw)
To: syzbot, ast, daniel, linux-kernel, netdev, syzkaller-bugs
In-Reply-To: <000000000000222dd6056d4c4dfc@google.com>
On 05/28/2018 04:16 PM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 7a1a98c171ea Merge branch 'bpf-sendmsg-hook'
> git tree: bpf-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=131f4067800000
> kernel config: https://syzkaller.appspot.com/x/.config?x=e4078980b886800c
> dashboard link: https://syzkaller.appspot.com/bug?extid=83bdee62c80cc044cb1a
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
> syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=17a0be2f800000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=164cf10f800000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+83bdee62c80cc044cb1a@syzkaller.appspotmail.com
>
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.17.0-rc6+ #25 Not tainted
> ------------------------------------------------------
[...]
#syz fix: bpf: sockhash fix omitted bucket lock in sock_close
^ permalink raw reply
* Re: possible deadlock in bpf_tcp_close
From: John Fastabend @ 2018-07-02 18:52 UTC (permalink / raw)
To: syzbot, ast, daniel, netdev, syzkaller-bugs
In-Reply-To: <000000000000ca8a42056d4530a5@google.com>
On 05/28/2018 07:47 AM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 7a1a98c171ea Merge branch 'bpf-sendmsg-hook'
> git tree: bpf-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=10fd82d7800000
> kernel config: https://syzkaller.appspot.com/x/.config?x=e4078980b886800c
> dashboard link: https://syzkaller.appspot.com/bug?extid=47ed903f50684f046b15
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>
> Unfortunately, I don't have any reproducer for this crash yet.
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+47ed903f50684f046b15@syzkaller.appspotmail.com
>
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 4.17.0-rc6+ #25 Not tainted
> ------------------------------------------------------
#syz fix: bpf: sockhash fix omitted bucket lock in sock_close
^ permalink raw reply
* Re: [PATCH] 6lowpan: iphc: reset mac_header after decompress to fix panic
From: Alexander Aring @ 2018-07-02 18:54 UTC (permalink / raw)
To: Michael Scott
Cc: Jukka Rissanen, David S. Miller, linux-bluetooth, linux-wpan,
netdev, linux-kernel
In-Reply-To: <20180619234406.8217-1-michael@opensourcefoundries.com>
Hi,
On Tue, Jun 19, 2018 at 04:44:06PM -0700, Michael Scott wrote:
> After decompression of 6lowpan socket data, an IPv6 header is inserted
> before the existing socket payload. After this, we reset the
> network_header value of the skb to account for the difference in payload
> size from prior to decompression + the addition of the IPv6 header.
>
> However, we fail to reset the mac_header value.
>
> Leaving the mac_header value untouched here, can cause a calculation
> error in net/packet/af_packet.c packet_rcv() function when an
> AF_PACKET socket is opened in SOCK_RAW mode for use on a 6lowpan
> interface.
>
> On line 2088, the data pointer is moved backward by the value returned
> from skb_mac_header(). If skb->data is adjusted so that it is before
> the skb->head pointer (which can happen when an old value of mac_header
> is left in place) the kernel generates a panic in net/core/skbuff.c
> line 1717.
>
> This panic can be generated by BLE 6lowpan interfaces (such as bt0) and
> 802.15.4 interfaces (such as lowpan0) as they both use the same 6lowpan
> sources for compression and decompression.
>
> Signed-off-by: Michael Scott <michael@opensourcefoundries.com>
> ---
> net/6lowpan/iphc.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
> index 6b1042e21656..52fad5dad9f7 100644
> --- a/net/6lowpan/iphc.c
> +++ b/net/6lowpan/iphc.c
> @@ -770,6 +770,7 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
> hdr.hop_limit, &hdr.daddr);
>
> skb_push(skb, sizeof(hdr));
> + skb_reset_mac_header(skb);
> skb_reset_network_header(skb);
> skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>
I think it's good to make that if the mac_header gets a dangled pointer.
But we don't have a mac header at this point anymore...
There exists also some functionality that the MAC header is not set, I
suppose this can be usefuly for tun like interfaces e.g. RAW IP what we
have here.
skb_mac_header_was_set
which does:
return skb->mac_header != (typeof(skb->mac_header))~0U;
maybe we can set it as (typeof(skb->mac_header))~0U and then everything
will run as far the kernel will not crash anymore.
Question is for me: which upper layer wants access MAC header here on
receive path?
It cannot parsed anyhow because so far I know no upper layer can parse
at the moment 802.15.4 frames (which is a complex format). Maybe over
some header_ops callback?
- Alex
^ permalink raw reply
* Re: KASAN: use-after-free Write in bpf_tcp_close
From: John Fastabend @ 2018-07-02 18:55 UTC (permalink / raw)
To: syzbot, ast, daniel, netdev, syzkaller-bugs
In-Reply-To: <000000000000cb4149056d3587f5@google.com>
On 05/27/2018 01:06 PM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: ff4fb475cea8 Merge branch 'btf-uapi-cleanups'
> git tree: bpf-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=12b3d577800000
> kernel config: https://syzkaller.appspot.com/x/.config?x=b632d8e2c2ab2c1
> dashboard link: https://syzkaller.appspot.com/bug?extid=31025a5f3f7650081204
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
> syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=109a2f37800000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=171a727b800000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+31025a5f3f7650081204@syzkaller.appspotmail.com
>
> ==================================================================
> BUG: KASAN: use-after-free in cmpxchg_size include/asm-generic/atomic-instrumented.h:355 [inline]
> BUG: KASAN: use-after-free in bpf_tcp_close+0x6f5/0xf80 kernel/bpf/sockmap.c:265
> Write of size 8 at addr ffff8801ca277680 by task syz-executor749/9723
#syz fix: bpf: sockhash fix omitted bucket lock in sock_close
^ permalink raw reply
* Re: KASAN: use-after-free Read in bpf_tcp_close
From: John Fastabend @ 2018-07-02 18:56 UTC (permalink / raw)
To: syzbot, ast, daniel, netdev, syzkaller-bugs
In-Reply-To: <000000000000ac9069056d1806c4@google.com>
On 05/26/2018 01:54 AM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 3fb48d881dbe Merge branch 'bpf-fib-mtu-check'
> git tree: bpf-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=15fc1977800000
> kernel config: https://syzkaller.appspot.com/x/.config?x=b632d8e2c2ab2c1
> dashboard link: https://syzkaller.appspot.com/bug?extid=fce8f2462c403d02af98
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
> syzkaller repro:https://syzkaller.appspot.com/x/repro.syz?x=1310c857800000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17de7177800000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+fce8f2462c403d02af98@syzkaller.appspotmail.com
>
> ==================================================================
#syz fix: bpf: sockhash fix omitted bucket lock in sock_close
^ permalink raw reply
* Re: [net-next 01/12] net/mlx5e: Add UDP GSO support
From: Boris Pismenny @ 2018-07-02 19:17 UTC (permalink / raw)
To: Alexander Duyck, Willem de Bruijn
Cc: David Miller, Network Development, Saeed Mahameed, ogerlitz,
yossiku
In-Reply-To: <CAKgT0UdC2c04JagxW8S==-ymBfDZVd6f=7DcLUGjRqiiZA3BwA@mail.gmail.com>
On 7/2/2018 6:32 PM, Alexander Duyck wrote:
>
>
> On Mon, Jul 2, 2018 at 7:46 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com
> <mailto:willemdebruijn.kernel@gmail.com>> wrote:
>
> On Mon, Jul 2, 2018 at 9:34 AM Willem de Bruijn
> <willemdebruijn.kernel@gmail.com
> <mailto:willemdebruijn.kernel@gmail.com>> wrote:
> >
> > On Mon, Jul 2, 2018 at 1:30 AM Boris Pismenny
> <borisp@mellanox.com <mailto:borisp@mellanox.com>> wrote:
> > >
> > >
> > >
> > > On 7/2/2018 4:45 AM, Willem de Bruijn wrote:
> > > >>> I've noticed that we could get cleaner code in our driver
> if we remove
> > > >>> these two lines from net/ipv4/udp_offload.c:
> > > >>> if (skb_is_gso(segs))
> > > >>> mss *= skb_shinfo(segs)->gso_segs;
> > > >>>
> > > >>> I think that this is correct in case of GSO_PARTIAL
> segmentation for the
> > > >>> following reasons:
> > > >>> 1. After this change the UDP payload field is consistent
> with the IP
> > > >>> header payload length field. Currently, IPv4 length is 1500
> and UDP
> > > >>> total length is the full unsegmented length.
> > > >
> > > > How does this simplify the driver? Does it currently have to
> > > > change the udph->length field to the mss on the wire, because the
> > > > device only splits + replicates the headers + computes the csum?
> > >
> > > Yes, this is the code I have at the moment.
> > >
> > > The device's limitation is more subtle than this. It could
> adjust the
> > > length, but then the checksum would be wrong.
> >
> > I see. We do have to keep in mind other devices. Alexander's ixgbe
> > RFC patch does not have this logic, so that device must update the
> > field directly.
> >
> > https://patchwork.ozlabs.org/patch/908396/
>
> To be clear, I think it's fine to remove these two lines if it does
> not cause
> problems for the ixgbe. It's quite possible that that device sets
> the udp
> length field unconditionally, ignoring the previous value. In which
> case both
> devices will work after this change without additional driver logic.
>
>
> I would prefer we didn’t modify this. Otherwise we cannot cancel out the
> length from the partial checksum when the time comes. The ixgbe code was
> making use of it if I recall.
>
AFAIU, the ixgbe patch doesn't use this. Instead the length is obtained
by the following code for both TCP and UDP segmentation:
paylen = skb->len - l4_offset;
Could you please check to see if this is actually required?
^ permalink raw reply
* [PATCH net-next 00/10] r8169: add phylib support
From: Heiner Kallweit @ 2018-07-02 19:29 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
Now that all the basic refactoring has been done we can add phylib
support. This patch series was successfully tested on:
RTL8168h
RTL8168evl
RTL8169sb
Heiner Kallweit (10):
r8169: add basic phylib support
r8169: use phy_resume/phy_suspend
r8169: replace open-coded PHY soft reset with genphy_soft_reset
r8169: use phy_ethtool_(g|s)et_link_ksettings
r8169: use phy_ethtool_nway_reset
r8169: use phy_mii_ioctl
r8169: migrate speed_down function to phylib
r8169: remove rtl8169_set_speed_xmii
r8169: remove mii_if_info member from struct rtl8169_private
r8169: don't read chip phy status register
drivers/net/ethernet/realtek/Kconfig | 2 +-
drivers/net/ethernet/realtek/r8169.c | 454 +++++++++------------------
2 files changed, 152 insertions(+), 304 deletions(-)
--
2.18.0
^ permalink raw reply
* Re: [patch net-next v2 0/9] net: sched: introduce chain templates support with offloading to mlxsw
From: Cong Wang @ 2018-07-02 19:33 UTC (permalink / raw)
To: Jiri Pirko
Cc: sridhar.samudrala, David Ahern, Jamal Hadi Salim,
Linux Kernel Network Developers, David Miller, Jakub Kicinski,
Simon Horman, john.hurley, mlxsw
In-Reply-To: <20180630101218.GA2181@nanopsycho>
On Sat, Jun 30, 2018 at 3:13 AM Jiri Pirko <jiri@resnulli.us> wrote:
> Okay. So that would allow either create a chain or "chain with
> template". Once that is done, there would be no means to manipulate the
> template. One can only remove the chain.
>
> What about refounting? I think it would make sense that this implicit
> chain addition would take one reference. That means if later on the last
> filter is removed, the chain would stay there until user removes it by
> hand.
Yeah, it is very similar to tc actions. So you can take a look
at how tc actions are refcnt'ed.
^ permalink raw reply
* [PATCH net-next 01/10] r8169: add basic phylib support
From: Heiner Kallweit @ 2018-07-02 19:36 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Add basic phylib support to r8169. All now unneeded old PHY handling code
will be removed in subsequent patches.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/Kconfig | 1 +
drivers/net/ethernet/realtek/r8169.c | 146 +++++++++++++++++++++------
2 files changed, 115 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7c69f4c8..7fb1af1f 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -99,6 +99,7 @@ config R8169
depends on PCI
select FW_LOADER
select CRC32
+ select PHYLIB
select MII
---help---
Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f80ac894..7443b230 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -16,6 +16,7 @@
#include <linux/delay.h>
#include <linux/ethtool.h>
#include <linux/mii.h>
+#include <linux/phy.h>
#include <linux/if_vlan.h>
#include <linux/crc32.h>
#include <linux/in.h>
@@ -754,6 +755,7 @@ struct rtl8169_private {
} wk;
struct mii_if_info mii;
+ struct mii_bus *mii_bus;
dma_addr_t counters_phys_addr;
struct rtl8169_counters *counters;
struct rtl8169_tc_offsets tc_offset;
@@ -1444,11 +1446,6 @@ static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
}
-static unsigned int rtl8169_xmii_link_ok(struct rtl8169_private *tp)
-{
- return RTL_R8(tp, PHYstatus) & LinkStatus;
-}
-
static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
{
unsigned int val;
@@ -1513,25 +1510,6 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
}
}
-static void rtl8169_check_link_status(struct net_device *dev,
- struct rtl8169_private *tp)
-{
- struct device *d = tp_to_dev(tp);
-
- if (rtl8169_xmii_link_ok(tp)) {
- rtl_link_chg_patch(tp);
- /* This is to cancel a scheduled suspend if there's one. */
- pm_request_resume(d);
- netif_carrier_on(dev);
- if (net_ratelimit())
- netif_info(tp, ifup, dev, "link up\n");
- } else {
- netif_carrier_off(dev);
- netif_info(tp, ifdown, dev, "link down\n");
- pm_runtime_idle(d);
- }
-}
-
#define WAKE_ANY (WAKE_PHY | WAKE_MAGIC | WAKE_UCAST | WAKE_BCAST | WAKE_MCAST)
/* Currently we only enable WoL if explicitly told by userspace to circumvent
@@ -6228,7 +6206,6 @@ static void rtl_reset_work(struct rtl8169_private *tp)
napi_enable(&tp->napi);
rtl_hw_start(tp);
netif_wake_queue(dev);
- rtl8169_check_link_status(dev, tp);
}
static void rtl8169_tx_timeout(struct net_device *dev)
@@ -6845,7 +6822,7 @@ static void rtl_slow_event_work(struct rtl8169_private *tp)
rtl8169_pcierr_interrupt(dev);
if (status & LinkChg)
- rtl8169_check_link_status(dev, tp);
+ phy_mac_interrupt(dev->phydev);
rtl_irq_enable_all(tp);
}
@@ -6927,10 +6904,53 @@ static void rtl8169_rx_missed(struct net_device *dev)
RTL_W32(tp, RxMissed, 0);
}
+static void r8169_phylink_handler(struct net_device *ndev)
+{
+ struct rtl8169_private *tp = netdev_priv(ndev);
+
+ if (netif_carrier_ok(ndev)) {
+ rtl_link_chg_patch(tp);
+ pm_request_resume(&tp->pci_dev->dev);
+ } else {
+ pm_runtime_idle(&tp->pci_dev->dev);
+ }
+
+ if (net_ratelimit())
+ phy_print_status(ndev->phydev);
+}
+
+static int r8169_phy_connect(struct rtl8169_private *tp)
+{
+ struct phy_device *phydev;
+ phy_interface_t phy_mode;
+ int ret;
+
+ phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+ PHY_INTERFACE_MODE_MII;
+
+ phydev = mdiobus_get_phy(tp->mii_bus, 0);
+ if (!phydev)
+ return -ENODEV;
+
+ if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+ netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
+ phy_set_max_speed(phydev, SPEED_100);
+ }
+
+ ret = phy_connect_direct(tp->dev, phydev, r8169_phylink_handler,
+ phy_mode);
+ if (!ret)
+ phy_attached_info(phydev);
+
+ return ret;
+}
+
static void rtl8169_down(struct net_device *dev)
{
struct rtl8169_private *tp = netdev_priv(dev);
+ phy_stop(dev->phydev);
+
napi_disable(&tp->napi);
netif_stop_queue(dev);
@@ -6970,6 +6990,8 @@ static int rtl8169_close(struct net_device *dev)
cancel_work_sync(&tp->wk.work);
+ phy_disconnect(dev->phydev);
+
pci_free_irq(pdev, 0, tp);
dma_free_coherent(&pdev->dev, R8169_RX_RING_BYTES, tp->RxDescArray,
@@ -7030,6 +7052,10 @@ static int rtl_open(struct net_device *dev)
if (retval < 0)
goto err_release_fw_2;
+ retval = r8169_phy_connect(tp);
+ if (retval)
+ goto err_free_irq;
+
rtl_lock_work(tp);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7045,16 +7071,17 @@ static int rtl_open(struct net_device *dev)
if (!rtl8169_init_counter_offsets(tp))
netif_warn(tp, hw, dev, "counter reset/update failed\n");
+ phy_start(dev->phydev);
netif_start_queue(dev);
rtl_unlock_work(tp);
pm_runtime_put_sync(&pdev->dev);
-
- rtl8169_check_link_status(dev, tp);
out:
return retval;
+err_free_irq:
+ pci_free_irq(pdev, 0, tp);
err_release_fw_2:
rtl_release_firmware(tp);
rtl8169_rx_clear(tp);
@@ -7133,6 +7160,7 @@ static void rtl8169_net_suspend(struct net_device *dev)
if (!netif_running(dev))
return;
+ phy_stop(dev->phydev);
netif_device_detach(dev);
netif_stop_queue(dev);
@@ -7165,6 +7193,8 @@ static void __rtl8169_resume(struct net_device *dev)
rtl_pll_power_up(tp);
rtl8169_init_phy(dev, tp);
+ phy_start(tp->dev->phydev);
+
rtl_lock_work(tp);
napi_enable(&tp->napi);
set_bit(RTL_FLAG_TASK_ENABLED, tp->wk.flags);
@@ -7310,6 +7340,7 @@ static void rtl_remove_one(struct pci_dev *pdev)
netif_napi_del(&tp->napi);
unregister_netdev(dev);
+ mdiobus_unregister(tp->mii_bus);
rtl_release_firmware(tp);
@@ -7395,6 +7426,51 @@ DECLARE_RTL_COND(rtl_rxtx_empty_cond)
return (RTL_R8(tp, MCU) & RXTX_EMPTY) == RXTX_EMPTY;
}
+static int r8169_mdio_read_reg(struct mii_bus *mii_bus, int phyaddr, int phyreg)
+{
+ struct rtl8169_private *tp = mii_bus->priv;
+
+ return rtl_readphy(tp, phyreg);
+}
+
+static int r8169_mdio_write_reg(struct mii_bus *mii_bus, int phyaddr,
+ int phyreg, u16 val)
+{
+ struct rtl8169_private *tp = mii_bus->priv;
+
+ rtl_writephy(tp, phyreg, val);
+
+ return 0;
+}
+
+static int r8169_mdio_register(struct rtl8169_private *tp)
+{
+ struct pci_dev *pdev = tp->pci_dev;
+ struct mii_bus *new_bus;
+ int ret;
+
+ new_bus = devm_mdiobus_alloc(&pdev->dev);
+ if (!new_bus)
+ return -ENOMEM;
+
+ new_bus->name = "r8169";
+ new_bus->phy_mask = ~1;
+ new_bus->priv = tp;
+ new_bus->parent = &pdev->dev;
+ new_bus->irq[0] = PHY_IGNORE_INTERRUPT;
+ snprintf(new_bus->id, MII_BUS_ID_SIZE, "r8169-%x",
+ PCI_DEVID(pdev->bus->number, pdev->devfn));
+
+ new_bus->read = r8169_mdio_read_reg;
+ new_bus->write = r8169_mdio_write_reg;
+
+ ret = mdiobus_register(new_bus);
+ if (!ret)
+ tp->mii_bus = new_bus;
+
+ return ret;
+}
+
static void rtl_hw_init_8168g(struct rtl8169_private *tp)
{
u32 data;
@@ -7651,10 +7727,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
pci_set_drvdata(pdev, dev);
- rc = register_netdev(dev);
- if (rc < 0)
+ rc = r8169_mdio_register(tp);
+ if (rc)
return rc;
+ rc = register_netdev(dev);
+ if (rc)
+ goto err_mdio_unregister;
+
netif_info(tp, probe, dev, "%s, %pM, XID %08x, IRQ %d\n",
rtl_chip_infos[chipset].name, dev->dev_addr,
(u32)(RTL_R32(tp, TxConfig) & 0xfcf0f8ff),
@@ -7669,12 +7749,14 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
if (r8168_check_dash(tp))
rtl8168_driver_start(tp);
- netif_carrier_off(dev);
-
if (pci_dev_run_wake(pdev))
pm_runtime_put_sync(&pdev->dev);
return 0;
+
+err_mdio_unregister:
+ mdiobus_unregister(tp->mii_bus);
+ return rc;
}
static struct pci_driver rtl8169_pci_driver = {
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 02/10] r8169: use phy_resume/phy_suspend
From: Heiner Kallweit @ 2018-07-02 19:36 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Use phy_resume() / phy_suspend() instead of open coding this functionality.
The chip version specific differences are handled by the respective PHY
drivers.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 48 +++-------------------------
1 file changed, 5 insertions(+), 43 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7443b230..0fba2581 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4457,47 +4457,6 @@ static bool rtl_wol_pll_power_down(struct rtl8169_private *tp)
return true;
}
-static void r8168_phy_power_up(struct rtl8169_private *tp)
-{
- rtl_writephy(tp, 0x1f, 0x0000);
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_11:
- case RTL_GIGA_MAC_VER_12:
- case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- rtl_writephy(tp, 0x0e, 0x0000);
- break;
- default:
- break;
- }
- rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE);
-
- /* give MAC/PHY some time to resume */
- msleep(20);
-}
-
-static void r8168_phy_power_down(struct rtl8169_private *tp)
-{
- rtl_writephy(tp, 0x1f, 0x0000);
- switch (tp->mac_version) {
- case RTL_GIGA_MAC_VER_32:
- case RTL_GIGA_MAC_VER_33:
- case RTL_GIGA_MAC_VER_40:
- case RTL_GIGA_MAC_VER_41:
- rtl_writephy(tp, MII_BMCR, BMCR_ANENABLE | BMCR_PDOWN);
- break;
-
- case RTL_GIGA_MAC_VER_11:
- case RTL_GIGA_MAC_VER_12:
- case RTL_GIGA_MAC_VER_17 ... RTL_GIGA_MAC_VER_28:
- case RTL_GIGA_MAC_VER_31:
- rtl_writephy(tp, 0x0e, 0x0200);
- default:
- rtl_writephy(tp, MII_BMCR, BMCR_PDOWN);
- break;
- }
-}
-
static void r8168_pll_power_down(struct rtl8169_private *tp)
{
if (r8168_check_dash(tp))
@@ -4510,7 +4469,8 @@ static void r8168_pll_power_down(struct rtl8169_private *tp)
if (rtl_wol_pll_power_down(tp))
return;
- r8168_phy_power_down(tp);
+ /* cover the case that PHY isn't connected */
+ phy_suspend(mdiobus_get_phy(tp->mii_bus, 0));
switch (tp->mac_version) {
case RTL_GIGA_MAC_VER_25 ... RTL_GIGA_MAC_VER_33:
@@ -4563,7 +4523,9 @@ static void r8168_pll_power_up(struct rtl8169_private *tp)
break;
}
- r8168_phy_power_up(tp);
+ phy_resume(tp->dev->phydev);
+ /* give MAC/PHY some time to resume */
+ msleep(20);
}
static void rtl_pll_power_down(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 03/10] r8169: replace open-coded PHY soft reset with genphy_soft_reset
From: Heiner Kallweit @ 2018-07-02 19:36 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Use genphy_soft_reset() instead of open-coding a PHY soft reset. We have
to do an explicit PHY soft reset because some chips use the genphy driver
which uses a no-op as soft_reset callback.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 27 +--------------------------
1 file changed, 1 insertion(+), 26 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 0fba2581..a466647e 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1441,19 +1441,6 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
RTL_R8(tp, ChipCmd);
}
-static unsigned int rtl8169_xmii_reset_pending(struct rtl8169_private *tp)
-{
- return rtl_readphy(tp, MII_BMCR) & BMCR_RESET;
-}
-
-static void rtl8169_xmii_reset_enable(struct rtl8169_private *tp)
-{
- unsigned int val;
-
- val = rtl_readphy(tp, MII_BMCR) | BMCR_RESET;
- rtl_writephy(tp, MII_BMCR, val & 0xffff);
-}
-
static void rtl_link_chg_patch(struct rtl8169_private *tp)
{
struct net_device *dev = tp->dev;
@@ -4259,18 +4246,6 @@ static void rtl_schedule_task(struct rtl8169_private *tp, enum rtl_flag flag)
schedule_work(&tp->wk.work);
}
-DECLARE_RTL_COND(rtl_phy_reset_cond)
-{
- return rtl8169_xmii_reset_pending(tp);
-}
-
-static void rtl8169_phy_reset(struct net_device *dev,
- struct rtl8169_private *tp)
-{
- rtl8169_xmii_reset_enable(tp);
- rtl_msleep_loop_wait_low(tp, &rtl_phy_reset_cond, 1, 100);
-}
-
static bool rtl_tbi_enabled(struct rtl8169_private *tp)
{
return (tp->mac_version == RTL_GIGA_MAC_VER_01) &&
@@ -4301,7 +4276,7 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
}
- rtl8169_phy_reset(dev, tp);
+ genphy_soft_reset(dev->phydev);
rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 04/10] r8169: use phy_ethtool_(g|s)et_link_ksettings
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Use phy_ethtool_(g|s)et_link_ksettings() for the respective ethtool_ops
callbacks.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 35 +++-------------------------
1 file changed, 3 insertions(+), 32 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index a466647e..d3a909fb 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1816,35 +1816,6 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
__vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), swab16(opts2 & 0xffff));
}
-static int rtl8169_get_link_ksettings(struct net_device *dev,
- struct ethtool_link_ksettings *cmd)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- mii_ethtool_get_link_ksettings(&tp->mii, cmd);
-
- return 0;
-}
-
-static int rtl8169_set_link_ksettings(struct net_device *dev,
- const struct ethtool_link_ksettings *cmd)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
- int rc;
- u32 advertising;
-
- if (!ethtool_convert_link_mode_to_legacy_u32(&advertising,
- cmd->link_modes.advertising))
- return -EINVAL;
-
- rtl_lock_work(tp);
- rc = rtl8169_set_speed(dev, cmd->base.autoneg, cmd->base.speed,
- cmd->base.duplex, advertising);
- rtl_unlock_work(tp);
-
- return rc;
-}
-
static void rtl8169_get_regs(struct net_device *dev, struct ethtool_regs *regs,
void *p)
{
@@ -2099,7 +2070,7 @@ static const struct rtl_coalesce_info *rtl_coalesce_info(struct net_device *dev)
const struct rtl_coalesce_info *ci;
int rc;
- rc = rtl8169_get_link_ksettings(dev, &ecmd);
+ rc = phy_ethtool_get_link_ksettings(dev, &ecmd);
if (rc < 0)
return ERR_PTR(rc);
@@ -2258,8 +2229,8 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_ethtool_stats = rtl8169_get_ethtool_stats,
.get_ts_info = ethtool_op_get_ts_info,
.nway_reset = rtl8169_nway_reset,
- .get_link_ksettings = rtl8169_get_link_ksettings,
- .set_link_ksettings = rtl8169_set_link_ksettings,
+ .get_link_ksettings = phy_ethtool_get_link_ksettings,
+ .set_link_ksettings = phy_ethtool_set_link_ksettings,
};
static void rtl8169_get_mac_version(struct rtl8169_private *tp,
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 05/10] r8169: use phy_ethtool_nway_reset
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Switch to using phy_ethtool_nway_reset().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/Kconfig | 1 -
drivers/net/ethernet/realtek/r8169.c | 9 +--------
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/Kconfig b/drivers/net/ethernet/realtek/Kconfig
index 7fb1af1f..e1cd934c 100644
--- a/drivers/net/ethernet/realtek/Kconfig
+++ b/drivers/net/ethernet/realtek/Kconfig
@@ -100,7 +100,6 @@ config R8169
select FW_LOADER
select CRC32
select PHYLIB
- select MII
---help---
Say Y here if you have a Realtek 8169 PCI Gigabit Ethernet adapter.
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index d3a909fb..6006676b 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1991,13 +1991,6 @@ static void rtl8169_get_strings(struct net_device *dev, u32 stringset, u8 *data)
}
}
-static int rtl8169_nway_reset(struct net_device *dev)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- return mii_nway_restart(&tp->mii);
-}
-
/*
* Interrupt coalescing
*
@@ -2228,7 +2221,7 @@ static const struct ethtool_ops rtl8169_ethtool_ops = {
.get_sset_count = rtl8169_get_sset_count,
.get_ethtool_stats = rtl8169_get_ethtool_stats,
.get_ts_info = ethtool_op_get_ts_info,
- .nway_reset = rtl8169_nway_reset,
+ .nway_reset = phy_ethtool_nway_reset,
.get_link_ksettings = phy_ethtool_get_link_ksettings,
.set_link_ksettings = phy_ethtool_set_link_ksettings,
};
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 06/10] r8169: use phy_mii_ioctl
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Switch to using phy_mii_ioctl().
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 25 +++----------------------
1 file changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 6006676b..311321ee 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4290,31 +4290,12 @@ static int rtl_set_mac_address(struct net_device *dev, void *p)
return 0;
}
-static int rtl_xmii_ioctl(struct rtl8169_private *tp,
- struct mii_ioctl_data *data, int cmd)
-{
- switch (cmd) {
- case SIOCGMIIPHY:
- data->phy_id = 32; /* Internal PHY */
- return 0;
-
- case SIOCGMIIREG:
- data->val_out = rtl_readphy(tp, data->reg_num & 0x1f);
- return 0;
-
- case SIOCSMIIREG:
- rtl_writephy(tp, data->reg_num & 0x1f, data->val_in);
- return 0;
- }
- return -EOPNOTSUPP;
-}
-
static int rtl8169_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
- struct rtl8169_private *tp = netdev_priv(dev);
- struct mii_ioctl_data *data = if_mii(ifr);
+ if (!netif_running(dev))
+ return -ENODEV;
- return netif_running(dev) ? rtl_xmii_ioctl(tp, data, cmd) : -ENODEV;
+ return phy_mii_ioctl(dev->phydev, ifr, cmd);
}
static void rtl_init_mdio_ops(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 07/10] r8169: migrate speed_down function to phylib
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Change rtl_speed_down() to use phylib.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 33 +++++++++++++---------------
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 311321ee..807fbc75 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4240,6 +4240,10 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
}
+ /* We may have called rtl_speed_down before */
+ dev->phydev->advertising = dev->phydev->supported;
+ genphy_config_aneg(dev->phydev);
+
genphy_soft_reset(dev->phydev);
rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
@@ -4323,28 +4327,21 @@ static void rtl_init_mdio_ops(struct rtl8169_private *tp)
}
}
+#define BASET10 (ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full)
+#define BASET100 (ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full)
+#define BASET1000 (ADVERTISED_1000baseT_Half | ADVERTISED_1000baseT_Full)
+
static void rtl_speed_down(struct rtl8169_private *tp)
{
- u32 adv;
- int lpa;
+ struct phy_device *phydev = tp->dev->phydev;
+ u32 adv = phydev->lp_advertising & phydev->supported;
- rtl_writephy(tp, 0x1f, 0x0000);
- lpa = rtl_readphy(tp, MII_LPA);
+ if (adv & BASET10)
+ phydev->advertising &= ~(BASET100 | BASET1000);
+ else if (adv & BASET100)
+ phydev->advertising &= ~BASET1000;
- if (lpa & (LPA_10HALF | LPA_10FULL))
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full;
- else if (lpa & (LPA_100HALF | LPA_100FULL))
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full;
- else
- adv = ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
- (tp->mii.supports_gmii ?
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full : 0);
-
- rtl8169_set_speed(tp->dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
- adv);
+ genphy_config_aneg(phydev);
}
static void rtl_wol_suspend_quirk(struct rtl8169_private *tp)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 08/10] r8169: remove rtl8169_set_speed_xmii
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
We can remove rtl8169_set_speed_xmii() now that phylib handles all this.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 90 ----------------------------
1 file changed, 90 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 807fbc75..b696b83d 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1670,89 +1670,6 @@ static int rtl8169_get_regs_len(struct net_device *dev)
return R8169_REGS_SIZE;
}
-static int rtl8169_set_speed_xmii(struct net_device *dev,
- u8 autoneg, u16 speed, u8 duplex, u32 adv)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
- int giga_ctrl, bmcr;
- int rc = -EINVAL;
-
- rtl_writephy(tp, 0x1f, 0x0000);
-
- if (autoneg == AUTONEG_ENABLE) {
- int auto_nego;
-
- auto_nego = rtl_readphy(tp, MII_ADVERTISE);
- auto_nego &= ~(ADVERTISE_10HALF | ADVERTISE_10FULL |
- ADVERTISE_100HALF | ADVERTISE_100FULL);
-
- if (adv & ADVERTISED_10baseT_Half)
- auto_nego |= ADVERTISE_10HALF;
- if (adv & ADVERTISED_10baseT_Full)
- auto_nego |= ADVERTISE_10FULL;
- if (adv & ADVERTISED_100baseT_Half)
- auto_nego |= ADVERTISE_100HALF;
- if (adv & ADVERTISED_100baseT_Full)
- auto_nego |= ADVERTISE_100FULL;
-
- auto_nego |= ADVERTISE_PAUSE_CAP | ADVERTISE_PAUSE_ASYM;
-
- giga_ctrl = rtl_readphy(tp, MII_CTRL1000);
- giga_ctrl &= ~(ADVERTISE_1000FULL | ADVERTISE_1000HALF);
-
- /* The 8100e/8101e/8102e do Fast Ethernet only. */
- if (tp->mii.supports_gmii) {
- if (adv & ADVERTISED_1000baseT_Half)
- giga_ctrl |= ADVERTISE_1000HALF;
- if (adv & ADVERTISED_1000baseT_Full)
- giga_ctrl |= ADVERTISE_1000FULL;
- } else if (adv & (ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full)) {
- netif_info(tp, link, dev,
- "PHY does not support 1000Mbps\n");
- goto out;
- }
-
- bmcr = BMCR_ANENABLE | BMCR_ANRESTART;
-
- rtl_writephy(tp, MII_ADVERTISE, auto_nego);
- rtl_writephy(tp, MII_CTRL1000, giga_ctrl);
- } else {
- if (speed == SPEED_10)
- bmcr = 0;
- else if (speed == SPEED_100)
- bmcr = BMCR_SPEED100;
- else
- goto out;
-
- if (duplex == DUPLEX_FULL)
- bmcr |= BMCR_FULLDPLX;
- }
-
- rtl_writephy(tp, MII_BMCR, bmcr);
-
- if (tp->mac_version == RTL_GIGA_MAC_VER_02 ||
- tp->mac_version == RTL_GIGA_MAC_VER_03) {
- if ((speed == SPEED_100) && (autoneg != AUTONEG_ENABLE)) {
- rtl_writephy(tp, 0x17, 0x2138);
- rtl_writephy(tp, 0x0e, 0x0260);
- } else {
- rtl_writephy(tp, 0x17, 0x2108);
- rtl_writephy(tp, 0x0e, 0x0000);
- }
- }
-
- rc = 0;
-out:
- return rc;
-}
-
-static int rtl8169_set_speed(struct net_device *dev,
- u8 autoneg, u16 speed, u8 duplex, u32 advertising)
-{
- return rtl8169_set_speed_xmii(dev, autoneg, speed, duplex, advertising);
-}
-
static netdev_features_t rtl8169_fix_features(struct net_device *dev,
netdev_features_t features)
{
@@ -4245,13 +4162,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
genphy_config_aneg(dev->phydev);
genphy_soft_reset(dev->phydev);
-
- rtl8169_set_speed(dev, AUTONEG_ENABLE, SPEED_1000, DUPLEX_FULL,
- ADVERTISED_10baseT_Half | ADVERTISED_10baseT_Full |
- ADVERTISED_100baseT_Half | ADVERTISED_100baseT_Full |
- (tp->mii.supports_gmii ?
- ADVERTISED_1000baseT_Half |
- ADVERTISED_1000baseT_Full : 0));
}
static void rtl_rar_set(struct rtl8169_private *tp, u8 *addr)
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 09/10] r8169: remove mii_if_info member from struct rtl8169_private
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
The only remaining usage of the struct mii_if_info member is to store the
information whether the chip is GMII-capable. So we can replace it with
a simple flag.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 38 +++++-----------------------
1 file changed, 7 insertions(+), 31 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index b696b83d..48c0e77c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -15,7 +15,6 @@
#include <linux/etherdevice.h>
#include <linux/delay.h>
#include <linux/ethtool.h>
-#include <linux/mii.h>
#include <linux/phy.h>
#include <linux/if_vlan.h>
#include <linux/crc32.h>
@@ -754,7 +753,7 @@ struct rtl8169_private {
struct work_struct work;
} wk;
- struct mii_if_info mii;
+ unsigned supports_gmii:1;
struct mii_bus *mii_bus;
dma_addr_t counters_phys_addr;
struct rtl8169_counters *counters;
@@ -1106,21 +1105,6 @@ static void rtl_w0w1_phy(struct rtl8169_private *tp, int reg_addr, int p, int m)
rtl_writephy(tp, reg_addr, (val & ~m) | p);
}
-static void rtl_mdio_write(struct net_device *dev, int phy_id, int location,
- int val)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- rtl_writephy(tp, location, val);
-}
-
-static int rtl_mdio_read(struct net_device *dev, int phy_id, int location)
-{
- struct rtl8169_private *tp = netdev_priv(dev);
-
- return rtl_readphy(tp, location);
-}
-
DECLARE_RTL_COND(rtl_ephyar_cond)
{
return RTL_R32(tp, EPHYAR) & EPHYAR_FLAG;
@@ -2253,15 +2237,15 @@ static void rtl8169_get_mac_version(struct rtl8169_private *tp,
"unknown MAC, using family default\n");
tp->mac_version = default_version;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_42) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_42 :
RTL_GIGA_MAC_VER_43;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_45) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_45 :
RTL_GIGA_MAC_VER_47;
} else if (tp->mac_version == RTL_GIGA_MAC_VER_46) {
- tp->mac_version = tp->mii.supports_gmii ?
+ tp->mac_version = tp->supports_gmii ?
RTL_GIGA_MAC_VER_46 :
RTL_GIGA_MAC_VER_48;
}
@@ -6714,14 +6698,14 @@ static int r8169_phy_connect(struct rtl8169_private *tp)
phy_interface_t phy_mode;
int ret;
- phy_mode = tp->mii.supports_gmii ? PHY_INTERFACE_MODE_GMII :
+ phy_mode = tp->supports_gmii ? PHY_INTERFACE_MODE_GMII :
PHY_INTERFACE_MODE_MII;
phydev = mdiobus_get_phy(tp->mii_bus, 0);
if (!phydev)
return -ENODEV;
- if (!tp->mii.supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
+ if (!tp->supports_gmii && phydev->supported & PHY_1000BT_FEATURES) {
netif_info(tp, probe, tp->dev, "Restrict PHY to 100Mbit because MAC doesn't support 1GBit\n");
phy_set_max_speed(phydev, SPEED_100);
}
@@ -7317,7 +7301,6 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
const struct rtl_cfg_info *cfg = rtl_cfg_infos + ent->driver_data;
struct rtl8169_private *tp;
- struct mii_if_info *mii;
struct net_device *dev;
int chipset, region, i;
int rc;
@@ -7337,14 +7320,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
tp->dev = dev;
tp->pci_dev = pdev;
tp->msg_enable = netif_msg_init(debug.msg_enable, R8169_MSG_DEFAULT);
-
- mii = &tp->mii;
- mii->dev = dev;
- mii->mdio_read = rtl_mdio_read;
- mii->mdio_write = rtl_mdio_write;
- mii->phy_id_mask = 0x1f;
- mii->reg_num_mask = 0x1f;
- mii->supports_gmii = cfg->has_gmii;
+ tp->supports_gmii = cfg->has_gmii;
/* enable device (incl. PCI PM wakeup and hotplug setup) */
rc = pcim_enable_device(pdev);
--
2.18.0
^ permalink raw reply related
* [PATCH net-next 10/10] r8169: don't read chip phy status register
From: Heiner Kallweit @ 2018-07-02 19:37 UTC (permalink / raw)
To: David Miller, Florian Fainelli, Andrew Lunn,
Realtek linux nic maintainers
Cc: netdev@vger.kernel.org
In-Reply-To: <096a5326-963c-9bef-6218-29fcde004111@gmail.com>
Instead of accessing the PHYstatus register we can use the information
phylib stores in the phy_device structure.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 48c0e77c..7b7de596 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -1428,18 +1428,19 @@ static void rtl8169_irq_mask_and_ack(struct rtl8169_private *tp)
static void rtl_link_chg_patch(struct rtl8169_private *tp)
{
struct net_device *dev = tp->dev;
+ struct phy_device *phydev = dev->phydev;
if (!netif_running(dev))
return;
if (tp->mac_version == RTL_GIGA_MAC_VER_34 ||
tp->mac_version == RTL_GIGA_MAC_VER_38) {
- if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+ if (phydev->speed == SPEED_1000) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
ERIAR_EXGMAC);
- } else if (RTL_R8(tp, PHYstatus) & _100bps) {
+ } else if (phydev->speed == SPEED_100) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x0000001f,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1457,7 +1458,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
ERIAR_EXGMAC);
} else if (tp->mac_version == RTL_GIGA_MAC_VER_35 ||
tp->mac_version == RTL_GIGA_MAC_VER_36) {
- if (RTL_R8(tp, PHYstatus) & _1000bpsF) {
+ if (phydev->speed == SPEED_1000) {
rtl_eri_write(tp, 0x1bc, ERIAR_MASK_1111, 0x00000011,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_1111, 0x00000005,
@@ -1469,7 +1470,7 @@ static void rtl_link_chg_patch(struct rtl8169_private *tp)
ERIAR_EXGMAC);
}
} else if (tp->mac_version == RTL_GIGA_MAC_VER_37) {
- if (RTL_R8(tp, PHYstatus) & _10bps) {
+ if (phydev->speed == SPEED_10) {
rtl_eri_write(tp, 0x1d0, ERIAR_MASK_0011, 0x4d02,
ERIAR_EXGMAC);
rtl_eri_write(tp, 0x1dc, ERIAR_MASK_0011, 0x0060,
--
2.18.0
^ permalink raw reply related
* Re: [PATCH] 6lowpan: iphc: reset mac_header after decompress to fix panic
From: Michael Scott @ 2018-07-02 19:45 UTC (permalink / raw)
To: Alexander Aring
Cc: Jukka Rissanen, David S. Miller, linux-bluetooth, linux-wpan,
netdev, linux-kernel
In-Reply-To: <20180702185438.dqqjg6k45iefj5is@x220t>
Hello Alexander,
On 07/02/2018 11:54 AM, Alexander Aring wrote:
> Hi,
>
> On Tue, Jun 19, 2018 at 04:44:06PM -0700, Michael Scott wrote:
>> After decompression of 6lowpan socket data, an IPv6 header is inserted
>> before the existing socket payload. After this, we reset the
>> network_header value of the skb to account for the difference in payload
>> size from prior to decompression + the addition of the IPv6 header.
>>
>> However, we fail to reset the mac_header value.
>>
>> Leaving the mac_header value untouched here, can cause a calculation
>> error in net/packet/af_packet.c packet_rcv() function when an
>> AF_PACKET socket is opened in SOCK_RAW mode for use on a 6lowpan
>> interface.
>>
>> On line 2088, the data pointer is moved backward by the value returned
>> from skb_mac_header(). If skb->data is adjusted so that it is before
>> the skb->head pointer (which can happen when an old value of mac_header
>> is left in place) the kernel generates a panic in net/core/skbuff.c
>> line 1717.
>>
>> This panic can be generated by BLE 6lowpan interfaces (such as bt0) and
>> 802.15.4 interfaces (such as lowpan0) as they both use the same 6lowpan
>> sources for compression and decompression.
>>
>> Signed-off-by: Michael Scott <michael@opensourcefoundries.com>
>> ---
>> net/6lowpan/iphc.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/6lowpan/iphc.c b/net/6lowpan/iphc.c
>> index 6b1042e21656..52fad5dad9f7 100644
>> --- a/net/6lowpan/iphc.c
>> +++ b/net/6lowpan/iphc.c
>> @@ -770,6 +770,7 @@ int lowpan_header_decompress(struct sk_buff *skb, const struct net_device *dev,
>> hdr.hop_limit, &hdr.daddr);
>>
>> skb_push(skb, sizeof(hdr));
>> + skb_reset_mac_header(skb);
>> skb_reset_network_header(skb);
>> skb_copy_to_linear_data(skb, &hdr, sizeof(hdr));
>>
> I think it's good to make that if the mac_header gets a dangled pointer.
> But we don't have a mac header at this point anymore...
>
> There exists also some functionality that the MAC header is not set, I
> suppose this can be usefuly for tun like interfaces e.g. RAW IP what we
> have here.
>
> skb_mac_header_was_set
>
> which does:
>
> return skb->mac_header != (typeof(skb->mac_header))~0U;
>
> maybe we can set it as (typeof(skb->mac_header))~0U and then everything
> will run as far the kernel will not crash anymore.
>
> Question is for me: which upper layer wants access MAC header here on
> receive path?
> It cannot parsed anyhow because so far I know no upper layer can parse
> at the moment 802.15.4 frames (which is a complex format). Maybe over
> some header_ops callback?
I was testing a C program which performs NAT64 handling on packets
destined to a certain IPv6 subnet (64:ff9b::). To do this, the
application opens a RAW socket like this: sniff_sock = socket(PF_PACKET,
SOCK_RAW, htons(ETH_P_ALL)); It then sets promiscuous mode and enters a
looping call of:
length = recv(sniff_sock, buffer, PACKET_BUFFER, MSG_TRUNC); My host PC
kernel would then promptly crash on me. (I'm going to purposely avoid
the obvious point of: this probably isn't the best way to parse packets
for NAT64 translation as you will get every single packet incoming or
outgoing on the host.) Turns out, testing the program on an 802.15.4
6lowpan interface exposed some of the issues which this mailing list
(but not myself) is well aware of (no L2 data in the RAW packets) and
also led me to debugging this patch to stop the kernel crash. TL;DR: To
summarize, any PF_PACKET SOCK_RAW socket which recv()'s IPv6 data from a
6lowpan node will cause this kernel crash eventually (checked on kernel
4.15, 4.16, 4.17 and 4.18-rc1). - Mike
>
> - Alex
^ permalink raw reply
* Regression introduced by "r8169: simplify rtl_set_mac_address"
From: Corinna Vinschen @ 2018-07-02 19:48 UTC (permalink / raw)
To: netdev, Heiner Kallweit
Hi,
the patch 1f7aa2bc268e, "r8169: simplify rtl_set_mac_address",
introduced a regression found by trying to team a r8169 NIC.
Try the following (assuming the r8169 NIC is eth0):
$ nmcli con add type team con-name team0 ifname nm-team config \
'{"runner": {"name": "lacp"}, "link_watch": {"name": "ethtool"}}' \
ipv4.method disable ipv6.method ignore
$ nmcli con add type ethernet ifname eth0 con-name team0.0 master nm-team
$ nmcli con up team0.0
$ teamdctl nm-team port present eth0
command call failed (No such device)
Bisecting turned up commit 1f7aa2bc268e, "r8169: simplify
rtl_set_mac_address" as the culprit. Reverting this patch fixes
the issue and the teamdctl call succeeds.
The reason is apparently the usage of eth_mac_addr here. eth_mac_addr
calls eth_prepare_mac_addr_change which checks for IFF_LIVE_ADDR_CHANGE.
Debugging shows this flag not being set on r8169, thus
eth_prepare_mac_addr_change returns -EBUSY (no idea why userspace claims
"No such device", rather than "Device or resource busy", but that's not
the point here).
Note that other devices like igb, don't call eth_mac_addr either, but
rather call memcpy by themselves to copy the new MAC, just as the
original r8169 code did, too. Consequentially this problem is not
present on igb.
I suggest to revert this change in the first place, but I wonder if
we're not just missing to set IFF_LIVE_ADDR_CHANGE in a lot of drivers.
Thanks,
Corinna
^ permalink raw reply
* [PATCH v2 net-next] openvswitch: kernel datapath clone action
From: Yifeng Sun @ 2018-07-02 15:18 UTC (permalink / raw)
To: pshelar, azhou, netdev; +Cc: Yifeng Sun
Add 'clone' action to kernel datapath by using existing functions.
When actions within clone don't modify the current flow, the flow
key is not cloned before executing clone actions.
This is a follow up patch for this incomplete work:
https://patchwork.ozlabs.org/patch/722096/
v1 -> v2:
Refactor as advised by reviewer.
Signed-off-by: Yifeng Sun <pkusunyifeng@gmail.com>
Signed-off-by: Andy Zhou <azhou@ovn.org>
---
include/linux/openvswitch.h | 5 +++
include/uapi/linux/openvswitch.h | 3 ++
net/openvswitch/actions.c | 33 ++++++++++++++++++
net/openvswitch/flow_netlink.c | 73 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 114 insertions(+)
diff --git a/include/linux/openvswitch.h b/include/linux/openvswitch.h
index e6b240b6..379affc 100644
--- a/include/linux/openvswitch.h
+++ b/include/linux/openvswitch.h
@@ -21,4 +21,9 @@
#include <uapi/linux/openvswitch.h>
+#define OVS_CLONE_ATTR_EXEC 0 /* Specify an u32 value. When nonzero,
+ * actions in clone will not change flow
+ * keys. False otherwise.
+ */
+
#endif /* _LINUX_OPENVSWITCH_H */
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 863aaba..dbe0cbe 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -840,6 +840,8 @@ struct ovs_action_push_eth {
* @OVS_ACTION_ATTR_POP_NSH: pop the outermost NSH header off the packet.
* @OVS_ACTION_ATTR_METER: Run packet through a meter, which may drop the
* packet, or modify the packet (e.g., change the DSCP field).
+ * @OVS_ACTION_ATTR_CLONE: make a copy of the packet and execute a list of
+ * actions without affecting the original packet and key.
*
* Only a single header can be set with a single %OVS_ACTION_ATTR_SET. Not all
* fields within a header are modifiable, e.g. the IPv4 protocol and fragment
@@ -873,6 +875,7 @@ enum ovs_action_attr {
OVS_ACTION_ATTR_PUSH_NSH, /* Nested OVS_NSH_KEY_ATTR_*. */
OVS_ACTION_ATTR_POP_NSH, /* No argument. */
OVS_ACTION_ATTR_METER, /* u32 meter ID. */
+ OVS_ACTION_ATTR_CLONE, /* Nested OVS_CLONE_ATTR_*. */
__OVS_ACTION_ATTR_MAX, /* Nothing past this will be accepted
* from userspace. */
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 30a5df2..85ae53d 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1057,6 +1057,28 @@ static int sample(struct datapath *dp, struct sk_buff *skb,
clone_flow_key);
}
+/* When 'last' is true, clone() should always consume the 'skb'.
+ * Otherwise, clone() should keep 'skb' intact regardless what
+ * actions are executed within clone().
+ */
+static int clone(struct datapath *dp, struct sk_buff *skb,
+ struct sw_flow_key *key, const struct nlattr *attr,
+ bool last)
+{
+ struct nlattr *actions;
+ struct nlattr *clone_arg;
+ int rem = nla_len(attr);
+ bool dont_clone_flow_key;
+
+ /* The first action is always 'OVS_CLONE_ATTR_ARG'. */
+ clone_arg = nla_data(attr);
+ dont_clone_flow_key = nla_get_u32(clone_arg);
+ actions = nla_next(clone_arg, &rem);
+
+ return clone_execute(dp, skb, key, 0, actions, rem, last,
+ !dont_clone_flow_key);
+}
+
static void execute_hash(struct sk_buff *skb, struct sw_flow_key *key,
const struct nlattr *attr)
{
@@ -1336,6 +1358,17 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
consume_skb(skb);
return 0;
}
+ break;
+
+ case OVS_ACTION_ATTR_CLONE: {
+ bool last = nla_is_last(a, rem);
+
+ err = clone(dp, skb, key, a, last);
+ if (last)
+ return err;
+
+ break;
+ }
}
if (unlikely(err)) {
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 391c407..a70097e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2460,6 +2460,40 @@ static int validate_and_copy_sample(struct net *net, const struct nlattr *attr,
return 0;
}
+static int validate_and_copy_clone(struct net *net,
+ const struct nlattr *attr,
+ const struct sw_flow_key *key,
+ struct sw_flow_actions **sfa,
+ __be16 eth_type, __be16 vlan_tci,
+ bool log, bool last)
+{
+ int start, err;
+ u32 exec;
+
+ if (nla_len(attr) && nla_len(attr) < NLA_HDRLEN)
+ return -EINVAL;
+
+ start = add_nested_action_start(sfa, OVS_ACTION_ATTR_CLONE, log);
+ if (start < 0)
+ return start;
+
+ exec = last || !actions_may_change_flow(attr);
+
+ err = ovs_nla_add_action(sfa, OVS_CLONE_ATTR_EXEC, &exec,
+ sizeof(exec), log);
+ if (err)
+ return err;
+
+ err = __ovs_nla_copy_actions(net, attr, key, sfa,
+ eth_type, vlan_tci, log);
+ if (err)
+ return err;
+
+ add_nested_action_end(*sfa, start);
+
+ return 0;
+}
+
void ovs_match_init(struct sw_flow_match *match,
struct sw_flow_key *key,
bool reset_key,
@@ -2849,6 +2883,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
[OVS_ACTION_ATTR_PUSH_NSH] = (u32)-1,
[OVS_ACTION_ATTR_POP_NSH] = 0,
[OVS_ACTION_ATTR_METER] = sizeof(u32),
+ [OVS_ACTION_ATTR_CLONE] = (u32)-1,
};
const struct ovs_action_push_vlan *vlan;
int type = nla_type(a);
@@ -3038,6 +3073,18 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
/* Non-existent meters are simply ignored. */
break;
+ case OVS_ACTION_ATTR_CLONE: {
+ bool last = nla_is_last(a, rem);
+
+ err = validate_and_copy_clone(net, a, key, sfa,
+ eth_type, vlan_tci,
+ log, last);
+ if (err)
+ return err;
+ skip_copy = true;
+ break;
+ }
+
default:
OVS_NLERR(log, "Unknown Action type %d", type);
return -EINVAL;
@@ -3116,6 +3163,26 @@ static int sample_action_to_attr(const struct nlattr *attr,
return err;
}
+static int clone_action_to_attr(const struct nlattr *attr,
+ struct sk_buff *skb)
+{
+ struct nlattr *start;
+ int err = 0, rem = nla_len(attr);
+
+ start = nla_nest_start(skb, OVS_ACTION_ATTR_CLONE);
+ if (!start)
+ return -EMSGSIZE;
+
+ err = ovs_nla_put_actions(nla_data(attr), rem, skb);
+
+ if (err)
+ nla_nest_cancel(skb, start);
+ else
+ nla_nest_end(skb, start);
+
+ return err;
+}
+
static int set_action_to_attr(const struct nlattr *a, struct sk_buff *skb)
{
const struct nlattr *ovs_key = nla_data(a);
@@ -3204,6 +3271,12 @@ int ovs_nla_put_actions(const struct nlattr *attr, int len, struct sk_buff *skb)
return err;
break;
+ case OVS_ACTION_ATTR_CLONE:
+ err = clone_action_to_attr(a, skb);
+ if (err)
+ return err;
+ break;
+
default:
if (nla_put(skb, type, nla_len(a), nla_data(a)))
return -EMSGSIZE;
--
2.7.4
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox