* Re: [PATCH v4 perf,bpf 15/15] perf, bpf: save information about short living bpf programs
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-16-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:19PM -0800, Song Liu wrote:
SNIP
> + btf_id = info_linear->info.btf_id;
> +
> + info_node = malloc(sizeof(struct bpf_prog_info_node));
> + if (info_node) {
> + info_node->info_linear = info_linear;
> + perf_env__insert_bpf_prog_info(env, info_node);
> + } else
> + free(info_linear);
> +
> + if (btf_id == 0)
> + goto out;
> +
> + if (btf__get_from_id(btf_id, &btf)) {
> + pr_debug("%s: failed to get BTF of id %u, aborting\n",
> + __func__, btf_id);
> + goto out;
> + }
> + perf_env__fetch_btf(env, btf_id, btf);
so is this the main reason we are doing this? getting the btf
data for bpf prog ids and store them?
please describe the whole bpf events/features data flow in
changelog as I asked in previous email
thanks,
jirka
^ permalink raw reply
* Re: [PATCH v4 perf,bpf 06/15] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-7-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:10PM -0800, Song Liu wrote:
> bpf_prog_info contains information necessary to annotate bpf programs.
> This patch saves bpf_prog_info for bpf programs loaded in the system.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> tools/perf/util/bpf-event.c | 32 +++++++++++++-
> tools/perf/util/bpf-event.h | 7 ++-
> tools/perf/util/env.c | 85 +++++++++++++++++++++++++++++++++++++
> tools/perf/util/env.h | 17 ++++++++
> 4 files changed, 139 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/bpf-event.c b/tools/perf/util/bpf-event.c
> index ff7ee149ec46..ce81b2c43a51 100644
> --- a/tools/perf/util/bpf-event.c
> +++ b/tools/perf/util/bpf-event.c
> @@ -10,6 +10,7 @@
> #include "debug.h"
> #include "symbol.h"
> #include "machine.h"
> +#include "env.h"
> #include "session.h"
>
> #define ptr_to_u64(ptr) ((__u64)(unsigned long)(ptr))
> @@ -54,17 +55,28 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
> struct bpf_event *bpf_event = &event->bpf_event;
> struct bpf_prog_info_linear *info_linear;
> struct perf_tool *tool = session->tool;
> + struct bpf_prog_info_node *info_node;
> struct bpf_prog_info *info;
> struct btf *btf = NULL;
> bool has_btf = false;
> + struct perf_env *env;
> u32 sub_prog_cnt, i;
> int err = 0;
> u64 arrays;
>
> + /*
> + * for perf-record and perf-report use header.env;
> + * otherwise, use global perf_env.
> + */
> + env = session->data ? &session->header.env : &perf_env;
> +
> arrays = 1UL << BPF_PROG_INFO_JITED_KSYMS;
> arrays |= 1UL << BPF_PROG_INFO_JITED_FUNC_LENS;
> arrays |= 1UL << BPF_PROG_INFO_FUNC_INFO;
> arrays |= 1UL << BPF_PROG_INFO_PROG_TAGS;
> + arrays |= 1UL << BPF_PROG_INFO_JITED_INSNS;
> + arrays |= 1UL << BPF_PROG_INFO_LINE_INFO;
> + arrays |= 1UL << BPF_PROG_INFO_JITED_LINE_INFO;
>
> info_linear = bpf_program__get_prog_info_linear(fd, arrays);
> if (IS_ERR_OR_NULL(info_linear)) {
> @@ -153,8 +165,8 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
> machine, process);
> }
>
> - /* Synthesize PERF_RECORD_BPF_EVENT */
> if (opts->bpf_event) {
> + /* Synthesize PERF_RECORD_BPF_EVENT */
> *bpf_event = (struct bpf_event){
> .header = {
> .type = PERF_RECORD_BPF_EVENT,
> @@ -167,6 +179,24 @@ static int perf_event__synthesize_one_bpf_prog(struct perf_session *session,
> memcpy(bpf_event->tag, info->tag, BPF_TAG_SIZE);
> memset((void *)event + event->header.size, 0, machine->id_hdr_size);
> event->header.size += machine->id_hdr_size;
> +
> + /* save bpf_prog_info to env */
why do we save this to perf_env in here? we just
synthesize the same data as event, so report and
top will read it and fill perf_env again, right?
could you please explain the whole flow of the
bpf events and its respective data in perf_env
and put it into the changelog
> + info_node = malloc(sizeof(struct bpf_prog_info_node));
> +
> + /*
> + * Do not bail out for !info_node, as we still want to
> + * call perf_tool__process_synth_event()
well, we are out of memory, so I dont think perf_tool__process_synth_event
will get too far.. also the perf_env data would be inconsistent with what
you store as event.. how can that work?
thanks,
jirka
> + */
> + if (info_node) {
> + info_node->info_linear = info_linear;
> + perf_env__insert_bpf_prog_info(env, info_node);
> + info_linear = NULL;
> + }
> +
SNIP
^ permalink raw reply
* Re: [PATCH v4 perf,bpf 06/15] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-7-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:10PM -0800, Song Liu wrote:
SNIP
> @@ -38,6 +116,12 @@ void perf_env__exit(struct perf_env *env)
> zfree(&env->memory_nodes);
> }
>
> +static void init_bpf_rb_trees(struct perf_env *env)
> +{
> + env->bpf_progs.prog_infos = RB_ROOT;
> + init_rwsem(&env->bpf_progs.lock);
> +}
> +
> int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
> {
> int i;
> @@ -59,6 +143,7 @@ int perf_env__set_cmdline(struct perf_env *env, int argc, const char *argv[])
>
> env->nr_cmdline = argc;
>
> + init_bpf_rb_trees(env);
this souldn't be in perf_env__set_cmdline,
it's there to set the cmdline
struct bpf_progs is first in perf_env that
needs other than zero initialization, so
I think we need to add perf_env__init function,
that will do that for all the paths that uses
perf_env
jirka
^ permalink raw reply
* Re: [PATCH v4 perf,bpf 06/15] perf, bpf: save bpf_prog_info in a rbtree in perf_env
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-7-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:10PM -0800, Song Liu wrote:
SNIP
> diff --git a/tools/perf/util/env.h b/tools/perf/util/env.h
> index d01b8355f4ca..33ef4b2d2a29 100644
> --- a/tools/perf/util/env.h
> +++ b/tools/perf/util/env.h
> @@ -3,7 +3,9 @@
> #define __PERF_ENV_H
>
> #include <linux/types.h>
> +#include <linux/rbtree.h>
> #include "cpumap.h"
> +#include "rwsem.h"
>
> struct cpu_topology_map {
> int socket_id;
> @@ -64,8 +66,19 @@ struct perf_env {
> struct memory_node *memory_nodes;
> unsigned long long memory_bsize;
> u64 clockid_res_ns;
> +
> + /*
> + * bpf_info_lock protects bpf rbtrees. This is needed because the
> + * trees are accessed by different threads in perf-top
> + */
> + struct {
> + struct rw_semaphore lock;
> + struct rb_root prog_infos;
could be just 'infos' ^^^^ is already in struct name
> + } bpf_progs;
> };
>
jirka
^ permalink raw reply
* Re: [PATCH v4 perf,bpf 12/15] perf, bpf: enable annotation of bpf program
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-13-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:16PM -0800, Song Liu wrote:
SNIP
> diff --git a/tools/perf/util/util.c b/tools/perf/util/util.c
> index 093352e93d50..0bc9ec69f38a 100644
> --- a/tools/perf/util/util.c
> +++ b/tools/perf/util/util.c
> @@ -506,3 +506,13 @@ const char *perf_tip(const char *dirpath)
>
> return tip;
> }
> +
> +void get_exec_path(char *tpath, size_t size)
> +{
> + const char *path = "/proc/self/exe";
> + ssize_t len;
> +
> + len = readlink(path, tpath, size - 1);
> + assert(len > 0);
> + tpath[len] = 0;
> +}
we have now helper for that, please rebase this on top of
arnaldo's perf/core:
94816add0005 perf tools: Add perf_exe() helper to find perf binary
jirka
^ permalink raw reply
* Re: [PATCH v4 perf,bpf 07/15] perf, bpf: save bpf_prog_info information as headers to perf.data
From: Jiri Olsa @ 2019-02-27 13:21 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-8-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:11PM -0800, Song Liu wrote:
SNIP
> + if (info_len > sizeof(struct bpf_prog_info)) {
> + pr_warning("detected invalid bpf_prog_info\n");
> + goto out;
> + }
> +
> + info_linear = malloc(sizeof(struct bpf_prog_info_linear) +
> + data_len);
> + if (!info_linear)
> + goto out;
> + info_linear->info_len = sizeof(struct bpf_prog_info);
> + info_linear->data_len = data_len;
> + if (do_read_u64(ff, (u64 *)(&info_linear->arrays)))
> + goto out;
> + if (__do_read(ff, &info_linear->info, info_len))
> + goto out;
> + if (info_len < sizeof(struct bpf_prog_info))
> + memset(((void *)(&info_linear->info)) + info_len, 0,
> + sizeof(struct bpf_prog_info) - info_len);
> +
> + if (__do_read(ff, info_linear->data, data_len))
> + goto out;
> +
> + /* endian mismatch, drop the info, continue */
so there's no way we can swap the data? why?
jirka
> + if (ff->ph->needs_swap) {
> + free(info_linear);
> + continue;
> + }
SNIP
^ permalink raw reply
* Re: [PATCH v4 perf,bpf 12/15] perf, bpf: enable annotation of bpf program
From: Jiri Olsa @ 2019-02-27 13:22 UTC (permalink / raw)
To: Song Liu
Cc: netdev, linux-kernel, ast, daniel, kernel-team, peterz, acme,
jolsa, namhyung
In-Reply-To: <20190226002019.3748539-13-songliubraving@fb.com>
On Mon, Feb 25, 2019 at 04:20:16PM -0800, Song Liu wrote:
SNIP
> + if (!opts->hide_src_code && srcline) {
> + args->offset = -1;
> + args->line = strdup(srcline);
> + args->line_nr = 0;
> + args->ms.sym = sym;
> + dl = disasm_line__new(args);
> + if (dl)
> + annotation_line__add(&dl->al,
> + ¬es->src->source);
please use { } around multiline if leg
jirka
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: dsa: microchip: add other KSZ9477 switch variants
From: Andrew Lunn @ 2019-02-27 13:23 UTC (permalink / raw)
To: Tristram.Ha; +Cc: Florian Fainelli, Pavel Machek, UNGLinuxDriver, netdev
In-Reply-To: <1551224265-9304-4-git-send-email-Tristram.Ha@microchip.com>
> + if (dev->dev->of_node) {
> + char name[80];
> +
> + /* KSZ8565 chip can only be set through device tree. */
> + if (!of_modalias_node(dev->dev->of_node, name, sizeof(name))) {
> + if (!strcmp(name, "ksz8565")) {
> + chip = KSZ8565_SW_CHIP;
> + id_hi = FAMILY_ID_98;
> + id_lo = 0x95;
> + }
> + }
of_device_is_compatible() seems like a better call use use.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: dsa: microchip: add other KSZ9477 switch variants
From: Andrew Lunn @ 2019-02-27 13:33 UTC (permalink / raw)
To: Tristram.Ha; +Cc: Florian Fainelli, Pavel Machek, UNGLinuxDriver, netdev
In-Reply-To: <1551224265-9304-4-git-send-email-Tristram.Ha@microchip.com>
On Tue, Feb 26, 2019 at 03:37:45PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
>
> Add other switches in KSZ9477 family.
>
> KSZ9896 is a switch with 6 ports; the last one is typically used to
> connect to MAC.
> KSZ9567 is same as KSZ9897 but with 1588 PTP capability.
> KSZ8567 is same as KSZ9567 but without gigabit capability.
> KSZ9563 is same as KSZ9893 but with 1588 PTP capability.
> KSZ8563 is same as KSZ9563 but without gigabit capability.
> KSZ8565 is a switch with 5 ports; however, port 7 has to be used to
> connect to MAC. This chip can only be set through device tree.
>
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
> drivers/net/dsa/microchip/ksz9477.c | 93 ++++++++++++++++++++++++++++++++-
> drivers/net/dsa/microchip/ksz9477_spi.c | 3 ++
> drivers/net/dsa/microchip/ksz_common.c | 4 ++
> 3 files changed, 98 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 3bb548a..81e7c2f 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -1264,6 +1264,32 @@ static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
> ksz_pread16(dev, port, REG_PORT_PHY_INT_ENABLE, &data16);
> }
>
> +#define KSZ_CHIP_NAME_SIZE 18
> +
> +static char *ksz9477_chip_names[KSZ_CHIP_NAME_SIZE] = {
> + "Microchip KSZ9477",
This looks wrong. You are defining an array of 18 elements, not an
array of pointers to 18 byte chars.
> + "Microchip KSZ9897",
> + "Microchip KSZ9896",
> + "Microchip KSZ9567",
> + "Microchip KSZ8567",
> + "Microchip KSZ8565",
> + "Microchip KSZ9893",
> + "Microchip KSZ9563",
> + "Microchip KSZ8563",
> +};
> +
> +enum {
> + KSZ9477_SW_CHIP,
> + KSZ9897_SW_CHIP,
> + KSZ9896_SW_CHIP,
> + KSZ9567_SW_CHIP,
> + KSZ8567_SW_CHIP,
> + KSZ8565_SW_CHIP,
> + KSZ9893_SW_CHIP,
> + KSZ9563_SW_CHIP,
> + KSZ8563_SW_CHIP,
> +};
There should be a one-to-one mapping between this enum and the array
above. You can make this clear using Designated Initializers.
> +
> static void ksz9477_config_cpu_port(struct dsa_switch *ds)
> {
> struct ksz_device *dev = ds->priv;
> @@ -1314,7 +1340,8 @@ static void ksz9477_config_cpu_port(struct dsa_switch *ds)
> p->vid_member = (1 << i);
> p->member = dev->port_mask;
> ksz9477_port_stp_state_set(ds, i, BR_STATE_DISABLED);
> - p->on = 1;
> + if (!dsa_is_unused_port(ds, i))
> + p->on = 1;
> if (i < dev->phy_port_cnt)
> p->phy = 1;
> if (dev->chip_id == 0x00947700 && i == 6) {
> @@ -1406,6 +1433,7 @@ static u32 ksz9477_get_port_addr(int port, int offset)
>
> static int ksz9477_switch_detect(struct ksz_device *dev)
> {
> + int chip = -1;
> u8 data8;
> u8 id_hi;
> u8 id_lo;
> @@ -1448,6 +1476,12 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
> dev->features &= ~GBIT_SUPPORT;
> dev->mib_port_cnt = 3;
> dev->phy_port_cnt = 2;
> + if (!(data8 & SW_AVB_ABLE))
> + chip = KSZ9893_SW_CHIP;
> + else if (data8 & SW_QW_ABLE)
> + chip = KSZ8563_SW_CHIP;
> + else
> + chip = KSZ9563_SW_CHIP;
> } else {
> /* Chip uses new XMII register definitions. */
> dev->features |= NEW_XMII;
> @@ -1455,6 +1489,37 @@ static int ksz9477_switch_detect(struct ksz_device *dev)
> /* Chip does not support gigabit. */
> if (!(data8 & SW_GIGABIT_ABLE))
> dev->features &= ~GBIT_SUPPORT;
> + if ((id_lo & 0xf) == 6)
> + dev->mib_port_cnt = 6;
> + if (id_hi == FAMILY_ID_94)
> + chip = KSZ9477_SW_CHIP;
> + else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_97)
> + chip = KSZ9897_SW_CHIP;
> + else if (id_hi == FAMILY_ID_98 && id_lo == CHIP_ID_96)
> + chip = KSZ9896_SW_CHIP;
> + else if (id_hi == FAMILY_ID_95 && id_lo == CHIP_ID_67)
> + chip = KSZ9567_SW_CHIP;
> + else if (id_hi == FAMILY_ID_85 && id_lo == CHIP_ID_67)
> + chip = KSZ8567_SW_CHIP;
> + if (id_lo == CHIP_ID_67) {
> + id_hi = FAMILY_ID_98;
> + id_lo = CHIP_ID_97;
> + } else if (id_lo == CHIP_ID_66) {
> + id_hi = FAMILY_ID_98;
> + id_lo = CHIP_ID_96;
> + }
Maybe add a mask to ksz_chip_data table, so you can mask id_low,
id_high and then compare against chip_id?
> @@ -1495,6 +1564,15 @@ struct ksz_chip_data {
> .port_cnt = 7, /* total physical port count */
> },
> {
> + .chip_id = 0x00989600,
> + .dev_name = "KSZ9896",
> + .num_vlans = 4096,
> + .num_alus = 4096,
> + .num_statics = 16,
> + .cpu_ports = 0x3F, /* can be configured as cpu port */
> + .port_cnt = 6, /* total port count */
> + },
Andrew
^ permalink raw reply
* Re: [PATCH iproute2-next] devlink: add support for updating device flash
From: Jiri Pirko @ 2019-02-27 13:44 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: dsahern, stephen, oss-drivers, netdev
In-Reply-To: <20190226202014.23626-1-jakub.kicinski@netronome.com>
Tue, Feb 26, 2019 at 09:20:14PM CET, jakub.kicinski@netronome.com wrote:
>Add new command for updating flash of devices via devlink API.
>Example:
>
>$ cp flash-boot.bin /lib/firmware/
>$ devlink dev flash pci/0000:05:00.0 file flash-boot.bin
>
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
Thanks!
>---
> devlink/devlink.c | 54 ++++++++++++++++++++++++++++++++++++++++++
> man/man8/devlink-dev.8 | 32 +++++++++++++++++++++++++
> 2 files changed, 86 insertions(+)
>
>diff --git a/devlink/devlink.c b/devlink/devlink.c
>index 960cdda99b5b..5c6cac1f76dd 100644
>--- a/devlink/devlink.c
>+++ b/devlink/devlink.c
>@@ -199,6 +199,8 @@ static void ifname_map_free(struct ifname_map *ifname_map)
> #define DL_OPT_REGION_SNAPSHOT_ID BIT(22)
> #define DL_OPT_REGION_ADDRESS BIT(23)
> #define DL_OPT_REGION_LENGTH BIT(24)
>+#define DL_OPT_FLASH_FILE_NAME BIT(25)
>+#define DL_OPT_FLASH_COMPONENT BIT(26)
>
> struct dl_opts {
> uint32_t present; /* flags of present items */
>@@ -230,6 +232,8 @@ struct dl_opts {
> uint32_t region_snapshot_id;
> uint64_t region_address;
> uint64_t region_length;
>+ const char *flash_file_name;
>+ const char *flash_component;
> };
>
> struct dl {
>@@ -1185,6 +1189,20 @@ static int dl_argv_parse(struct dl *dl, uint32_t o_required,
> if (err)
> return err;
> o_found |= DL_OPT_REGION_LENGTH;
>+ } else if (dl_argv_match(dl, "file") &&
>+ (o_all & DL_OPT_FLASH_FILE_NAME)) {
>+ dl_arg_inc(dl);
>+ err = dl_argv_str(dl, &opts->flash_file_name);
>+ if (err)
>+ return err;
>+ o_found |= DL_OPT_FLASH_FILE_NAME;
>+ } else if (dl_argv_match(dl, "component") &&
>+ (o_all & DL_OPT_FLASH_COMPONENT)) {
>+ dl_arg_inc(dl);
>+ err = dl_argv_str(dl, &opts->flash_component);
>+ if (err)
>+ return err;
>+ o_found |= DL_OPT_FLASH_COMPONENT;
> } else {
> pr_err("Unknown option \"%s\"\n", dl_argv(dl));
> return -EINVAL;
>@@ -1389,6 +1407,12 @@ static void dl_opts_put(struct nlmsghdr *nlh, struct dl *dl)
> if (opts->present & DL_OPT_REGION_LENGTH)
> mnl_attr_put_u64(nlh, DEVLINK_ATTR_REGION_CHUNK_LEN,
> opts->region_length);
>+ if (opts->present & DL_OPT_FLASH_FILE_NAME)
>+ mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_FILE_NAME,
>+ opts->flash_file_name);
>+ if (opts->present & DL_OPT_FLASH_COMPONENT)
>+ mnl_attr_put_strz(nlh, DEVLINK_ATTR_FLASH_UPDATE_COMPONENT,
>+ opts->flash_component);
> }
>
> static int dl_argv_parse_put(struct nlmsghdr *nlh, struct dl *dl,
>@@ -1451,6 +1475,7 @@ static void cmd_dev_help(void)
> pr_err(" devlink dev param show [DEV name PARAMETER]\n");
> pr_err(" devlink dev reload DEV\n");
> pr_err(" devlink dev info [ DEV ]\n");
>+ pr_err(" devlink dev flash DEV file PATH [ component NAME ]\n");
> }
>
> static bool cmp_arr_last_handle(struct dl *dl, const char *bus_name,
>@@ -2583,6 +2608,32 @@ static int cmd_dev_info(struct dl *dl)
> return err;
> }
>
>+static void cmd_dev_flash_help(void)
>+{
>+ pr_err("Usage: devlink dev flash DEV file PATH [ component NAME ]\n");
>+}
>+
>+static int cmd_dev_flash(struct dl *dl)
>+{
>+ struct nlmsghdr *nlh;
>+ int err;
>+
>+ if (dl_argv_match(dl, "help") || dl_no_arg(dl)) {
>+ cmd_dev_flash_help();
>+ return 0;
>+ }
>+
>+ nlh = mnlg_msg_prepare(dl->nlg, DEVLINK_CMD_FLASH_UPDATE,
>+ NLM_F_REQUEST | NLM_F_ACK);
>+
>+ err = dl_argv_parse_put(nlh, dl, DL_OPT_HANDLE | DL_OPT_FLASH_FILE_NAME,
>+ DL_OPT_FLASH_COMPONENT);
>+ if (err)
>+ return err;
>+
>+ return _mnlg_socket_sndrcv(dl->nlg, nlh, NULL, NULL);
>+}
>+
> static int cmd_dev(struct dl *dl)
> {
> if (dl_argv_match(dl, "help")) {
>@@ -2604,6 +2655,9 @@ static int cmd_dev(struct dl *dl)
> } else if (dl_argv_match(dl, "info")) {
> dl_arg_inc(dl);
> return cmd_dev_info(dl);
>+ } else if (dl_argv_match(dl, "flash")) {
>+ dl_arg_inc(dl);
>+ return cmd_dev_flash(dl);
> }
> pr_err("Command \"%s\" not found\n", dl_argv(dl));
> return -ENOENT;
>diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
>index 47838371fecd..1804463b2321 100644
>--- a/man/man8/devlink-dev.8
>+++ b/man/man8/devlink-dev.8
>@@ -69,6 +69,16 @@ devlink-dev \- devlink device configuration
> .IR DEV
> .RI "]"
>
>+.ti -8
>+.BR "devlink dev flash"
>+.IR DEV
>+.BR file
>+.IR PATH
>+.RI "["
>+.BR target
>+.IR ID
>+.RI "]"
>+
> .SH "DESCRIPTION"
> .SS devlink dev show - display devlink device attributes
>
>@@ -177,6 +187,28 @@ versions may differ after flash has been updated, but before reboot.
> - specifies the devlink device to show.
> If this argument is omitted all devices are listed.
>
>+.SS devlink dev flash - write device's non-volatile memory.
>+
>+.PP
>+.I "DEV"
>+- specifies the devlink device to write to.
>+
>+.BR file
>+.I PATH
>+- Path to the file which will be written into device's flash. The path needs
>+to be relative to one of the directories searched by the kernel firmware loaded,
>+such as /lib/firmware.
>+
>+.BR component
>+.I NAME
>+- If device stores multiple firmware images in non-volatile memory, this
>+parameter may be used to indicate which firmware image should be written.
>+The value of
>+.I NAME
>+should match the component names from
>+.B "devlink dev info"
>+and may be driver-dependent.
>+
> .SH "EXAMPLES"
> .PP
> devlink dev show
>--
>2.19.2
>
^ permalink raw reply
* [PATCH RESEND ethtool] ethtool: add 10000baseR_FEC link mode name
From: Michal Kubecek @ 2019-02-27 13:49 UTC (permalink / raw)
To: John W. Linville; +Cc: netdev
ETHTOOL_LINK_MODE_10000baseR_FEC_BIT link mode is listed in
ethtool_link_mode_bit_indices array but doesn't have its name assigned in
the table in dump_link_caps() (so that it's not shown in "ethtool <dev>"
output) and is not listed in the manual page. Add it to both.
Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
---
ethtool.8.in | 1 +
ethtool.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/ethtool.8.in b/ethtool.8.in
index 5a26cff5fb33..3d288eaadf23 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -633,6 +633,7 @@ lB l lB.
0x1000 10000baseT Full
0x40000 10000baseKX4 Full
0x80000 10000baseKR Full
+0x100000 10000baseR_FEC
0x40000000000 10000baseCR Full
0x80000000000 10000baseSR Full
0x100000000000 10000baseLR Full
diff --git a/ethtool.c b/ethtool.c
index fb4c0886ca84..b6a1eaa383a3 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -633,6 +633,8 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
"10000baseKX4/Full" },
{ 0, ETHTOOL_LINK_MODE_10000baseKR_Full_BIT,
"10000baseKR/Full" },
+ { 0, ETHTOOL_LINK_MODE_10000baseR_FEC_BIT,
+ "10000baseR_FEC" },
{ 0, ETHTOOL_LINK_MODE_20000baseMLD2_Full_BIT,
"20000baseMLD2/Full" },
{ 0, ETHTOOL_LINK_MODE_20000baseKR2_Full_BIT,
--
2.20.1
^ permalink raw reply related
* [PATCH net-next] Revert "net: sched: route: don't set arg->stop in route4_walk() when empty"
From: Vlad Buslov @ 2019-02-27 13:48 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
This reverts commit 3027ff41f67c ("net: sched: route: don't set arg->stop
in route4_walk() when empty")
Cls API function tcf_proto_is_empty() was changed in commit
6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
to no longer depend on arg->stop to determine that classifier instance is
empty. Instead, it adds dedicated arg->nonempty field, which makes the fix
in route classifier no longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/cls_route.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/sched/cls_route.c b/net/sched/cls_route.c
index f006af23b64a..319f68cc233f 100644
--- a/net/sched/cls_route.c
+++ b/net/sched/cls_route.c
@@ -567,7 +567,10 @@ static void route4_walk(struct tcf_proto *tp, struct tcf_walker *arg,
struct route4_head *head = rtnl_dereference(tp->root);
unsigned int h, h1;
- if (head == NULL || arg->stop)
+ if (head == NULL)
+ arg->stop = 1;
+
+ if (arg->stop)
return;
for (h = 0; h <= 256; h++) {
--
2.13.6
^ permalink raw reply related
* [PATCH net-next] Revert "net: sched: fw: don't set arg->stop in fw_walk() when empty"
From: Vlad Buslov @ 2019-02-27 13:49 UTC (permalink / raw)
To: netdev; +Cc: jhs, xiyou.wangcong, jiri, davem, Vlad Buslov
This reverts commit 31a998487641 ("net: sched: fw: don't set arg->stop in
fw_walk() when empty")
Cls API function tcf_proto_is_empty() was changed in commit
6676d5e416ee ("net: sched: set dedicated tcf_walker flag when tp is empty")
to no longer depend on arg->stop to determine that classifier instance is
empty. Instead, it adds dedicated arg->nonempty field, which makes the fix
in fw classifier no longer necessary.
Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
---
net/sched/cls_fw.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/sched/cls_fw.c b/net/sched/cls_fw.c
index 456ee6e62dfa..ad036b00427d 100644
--- a/net/sched/cls_fw.c
+++ b/net/sched/cls_fw.c
@@ -363,7 +363,10 @@ static void fw_walk(struct tcf_proto *tp, struct tcf_walker *arg,
struct fw_head *head = rtnl_dereference(tp->root);
int h;
- if (head == NULL || arg->stop)
+ if (head == NULL)
+ arg->stop = 1;
+
+ if (arg->stop)
return;
for (h = 0; h < HTSIZE; h++) {
--
2.13.6
^ permalink raw reply related
* Re: [PATCH][net-next] ethtool: Use explicit designated initializers for .cmd
From: Michal Kubecek @ 2019-02-27 13:51 UTC (permalink / raw)
To: Li RongQing; +Cc: netdev
In-Reply-To: <1551271677-14492-1-git-send-email-lirongqing@baidu.com>
On Wed, Feb 27, 2019 at 08:47:57PM +0800, Li RongQing wrote:
> Initialize the .cmd member by using a designated struct
> initializer. This fixes warning of missing field initializers,
> and makes code a little easier to read.
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
Reviewed-by: Michal Kubecek <mkubecek@suse.cz>
> ---
> net/core/ethtool.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/ethtool.c b/net/core/ethtool.c
> index 47558ffae423..d4918ffddda8 100644
> --- a/net/core/ethtool.c
> +++ b/net/core/ethtool.c
> @@ -1717,7 +1717,7 @@ 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 = { .cmd = ETHTOOL_GPAUSEPARAM };
>
> if (!dev->ethtool_ops->get_pauseparam)
> return -EOPNOTSUPP;
> @@ -2503,7 +2503,7 @@ static int set_phy_tunable(struct net_device *dev, void __user *useraddr)
>
> static int ethtool_get_fecparam(struct net_device *dev, void __user *useraddr)
> {
> - struct ethtool_fecparam fecparam = { ETHTOOL_GFECPARAM };
> + struct ethtool_fecparam fecparam = { .cmd = ETHTOOL_GFECPARAM };
> int rc;
>
> if (!dev->ethtool_ops->get_fecparam)
> --
> 2.16.2
>
^ permalink raw reply
* Re: [PATCH net-next v3 4/4] net/mlx5e: Return -EOPNOTSUPP when attempting to offload an unsupported action
From: Roi Dayan @ 2019-02-27 14:01 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, Saeed Mahameed, gerlitz.or@gmail.com
Cc: netdev@vger.kernel.org
In-Reply-To: <1551184063-40628-5-git-send-email-xiangxia.m.yue@gmail.com>
On 26/02/2019 14:27, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> * Now the encapsulation is not supported for mlx5 VFs. When we try to
> offload that action, the -EINVAL is returned, but not -EOPNOTSUPP.
> This patch changes the returned value and ignore to confuse user.
> The command is shown as below [1].
>
> * When max modify header action is zero, we return -EOPNOTSUPP
> directly. In this way, we can ignore wrong message info (e.g.
> "mlx5: parsed 0 pedit actions, can't do more"). This happens when
> offloading pedit actions on mlx(cx4) VFs. The command is shown as below [2].
>
> For example: (p2p1_0 is VF net device)
> [1]
> $ tc filter add dev p2p1_0 protocol ip parent ffff: prio 1 flower skip_sw \
> src_mac e4:11:22:33:44:01 \
> action tunnel_key set \
> src_ip 1.1.1.100 \
> dst_ip 1.1.1.200 \
> dst_port 4789 id 100 \
> action mirred egress redirect dev vxlan0
>
> [2]
> $ tc filter add dev p2p1_0 parent ffff: protocol ip prio 1 \
> flower skip_sw dst_mac 00:10:56:fb:64:e8 \
> dst_ip 1.1.1.100 src_ip 1.1.1.200 \
> action pedit ex munge eth src set 00:10:56:b4:5d:20
>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 19 ++++++++++++++++---
> 1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 56ac50d..3a02b22 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -2035,7 +2035,7 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
> struct netlink_ext_ack *extack)
> {
> u8 cmd = (act->id == FLOW_ACTION_MANGLE) ? 0 : 1;
> - int err = -EOPNOTSUPP;
> + int max_actions, err = -EOPNOTSUPP;
> u32 mask, val, offset;
> u8 htype;
>
> @@ -2047,6 +2047,17 @@ static int parse_tc_pedit_action(struct mlx5e_priv *priv,
> goto out_err;
> }
>
> + if (namespace == MLX5_FLOW_NAMESPACE_FDB) /* FDB offloading */
> + max_actions = MLX5_CAP_ESW_FLOWTABLE_FDB(priv->mdev, max_modify_header_actions);
> + else /* namespace is MLX5_FLOW_NAMESPACE_KERNEL - NIC offloading */
> + max_actions = MLX5_CAP_FLOWTABLE_NIC_RX(priv->mdev, max_modify_header_actions);
> +
we have the exact same block with comments in alloc_mod_hdr_actions() which
is called later after we parse the action and now parsing the pedit fields.
your check for max_actions was there in prev v. why move it?
if like this I suggest we create a small static function to get max actions
for a namespace and call it from both places to get max actions.
> + if (!max_actions) {
> + NL_SET_ERR_MSG_MOD(extack,
> + "don't support pedit actions, can't offload");
please rephrase the msg to be consistent
i.e. The pedit offload action is not supported
> + goto out_err;
> + }
> +
> mask = act->mangle.mask;
> val = act->mangle.val;
> offset = act->mangle.offset;
> @@ -2294,7 +2305,8 @@ static int parse_tc_nic_actions(struct mlx5e_priv *priv,
> }
> break;
> default:
> - return -EINVAL;
> + NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + return -EOPNOTSUPP;
> }
> }
>
> @@ -2616,7 +2628,8 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
> break;
> }
> default:
> - return -EINVAL;
> + NL_SET_ERR_MSG_MOD(extack, "The offload action is not supported");
> + return -EOPNOTSUPP;
> }
> }
>
>
^ permalink raw reply
* Re: [PATCH] can: flexcan: add TX support for variable payload size
From: Marc Kleine-Budde @ 2019-02-27 14:03 UTC (permalink / raw)
To: Joakim Zhang, linux-can@vger.kernel.org
Cc: wg@grandegger.com, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, dl-linux-imx
In-Reply-To: <20181212064419.18613-1-qiangqing.zhang@nxp.com>
[-- Attachment #1.1: Type: text/plain, Size: 4992 bytes --]
On 12/12/18 7:46 AM, Joakim Zhang wrote:
> Now the FlexCAN driver always use last mailbox for TX, it will work well
> when MB payload size is 8/16 bytes.
> TX mailbox would change to 13 when MB payload size is 64 bytes to
> support CANFD. So we may need to set iflag register to add support for
> variable payload size.
>
> Signed-off-by: Joakim Zhang <qiangqing.zhang@nxp.com>
> ---
> drivers/net/can/flexcan.c | 42 +++++++++++++++++++++++++++++----------
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index 0f36eafe3ac1..13fd085fcf84 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -141,7 +141,9 @@
> #define FLEXCAN_TX_MB_RESERVED_OFF_FIFO 8
> #define FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP 0
> #define FLEXCAN_RX_MB_OFF_TIMESTAMP_FIRST (FLEXCAN_TX_MB_RESERVED_OFF_TIMESTAMP + 1)
> -#define FLEXCAN_IFLAG_MB(x) BIT((x) & 0x1f)
> +#define FLEXCAN_IFLAG1_MB_NUM 32
> +#define FLEXCAN_IFLAG1_MB(x) BIT(x)
> +#define FLEXCAN_IFLAG2_MB(x) BIT((x) & 0x1f)
> #define FLEXCAN_IFLAG_RX_FIFO_OVERFLOW BIT(7)
> #define FLEXCAN_IFLAG_RX_FIFO_WARN BIT(6)
> #define FLEXCAN_IFLAG_RX_FIFO_AVAILABLE BIT(5)
> @@ -822,9 +824,15 @@ static inline u64 flexcan_read_reg_iflag_rx(struct flexcan_priv *priv)
> struct flexcan_regs __iomem *regs = priv->regs;
> u32 iflag1, iflag2;
>
> - iflag2 = priv->read(®s->iflag2) & priv->reg_imask2_default &
> - ~FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> - iflag1 = priv->read(®s->iflag1) & priv->reg_imask1_default;
> + if (priv->tx_mb_idx >= FLEXCAN_IFLAG1_MB_NUM) {
> + iflag2 = priv->read(®s->iflag2) & priv->reg_imask2_default &
> + ~FLEXCAN_IFLAG2_MB(priv->tx_mb_idx);
> + iflag1 = priv->read(®s->iflag1) & priv->reg_imask1_default;
> + } else {
> + iflag2 = priv->read(®s->iflag2) & priv->reg_imask2_default;
> + iflag1 = priv->read(®s->iflag1) & priv->reg_imask1_default &
> + ~FLEXCAN_IFLAG1_MB(priv->tx_mb_idx);
> + }
I just noticed, that FLEXCAN_IFLAGx_MB(priv->tx_mb_idx) should already
be part of the corresponding imaskx_default. See flexcan_open(). So we
can remove it completely here, right?
>
> return (u64)iflag2 << 32 | iflag1;
> }
> @@ -836,7 +844,8 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> struct flexcan_priv *priv = netdev_priv(dev);
> struct flexcan_regs __iomem *regs = priv->regs;
> irqreturn_t handled = IRQ_NONE;
> - u32 reg_iflag2, reg_esr;
> + u32 reg_tx_iflag, tx_iflag_idx, reg_esr;
"tx_iflag_idx" is not an index (going from 0...63) but a bit-mask.
> + void __iomem *reg_iflag;
"reg_iflag" is not a register but a pointer to a register, better rename
it. There is a "u64 reg_iflag" in the same function already, but in a
different scope. Why not make it a u32 instead of a void?
> enum can_state last_state = priv->can.state;
>
> /* reception interrupt */
> @@ -870,10 +879,18 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> }
> }
>
> - reg_iflag2 = priv->read(®s->iflag2);
> + if (priv->tx_mb_idx >= FLEXCAN_IFLAG1_MB_NUM) {
> + reg_tx_iflag = priv->read(®s->iflag2);
> + tx_iflag_idx = FLEXCAN_IFLAG2_MB(priv->tx_mb_idx);
> + reg_iflag = ®s->iflag2;
> + } else {
> + reg_tx_iflag = priv->read(®s->iflag1);
> + tx_iflag_idx = FLEXCAN_IFLAG1_MB(priv->tx_mb_idx);
> + reg_iflag = ®s->iflag1;
> + }
>
> /* transmission complete interrupt */
> - if (reg_iflag2 & FLEXCAN_IFLAG_MB(priv->tx_mb_idx)) {
> + if (reg_tx_iflag & tx_iflag_idx) {
> u32 reg_ctrl = priv->read(&priv->tx_mb->can_ctrl);
>
> handled = IRQ_HANDLED;
> @@ -885,7 +902,7 @@ static irqreturn_t flexcan_irq(int irq, void *dev_id)
> /* after sending a RTR frame MB is in RX mode */
> priv->write(FLEXCAN_MB_CODE_TX_INACTIVE,
> &priv->tx_mb->can_ctrl);
> - priv->write(FLEXCAN_IFLAG_MB(priv->tx_mb_idx), ®s->iflag2);
> + priv->write(tx_iflag_idx, reg_iflag);
> netif_wake_queue(dev);
> }
>
> @@ -1244,8 +1261,13 @@ static int flexcan_open(struct net_device *dev)
> priv->tx_mb_idx = priv->mb_count - 1;
> priv->tx_mb = flexcan_get_mb(priv, priv->tx_mb_idx);
>
> - priv->reg_imask1_default = 0;
> - priv->reg_imask2_default = FLEXCAN_IFLAG_MB(priv->tx_mb_idx);
> + if (priv->tx_mb_idx >= FLEXCAN_IFLAG1_MB_NUM) {
> + priv->reg_imask1_default = 0;
> + priv->reg_imask2_default = FLEXCAN_IFLAG2_MB(priv->tx_mb_idx);
> + } else {
> + priv->reg_imask1_default = FLEXCAN_IFLAG1_MB(priv->tx_mb_idx);
> + priv->reg_imask2_default = 0;
> + }
>
> priv->offload.mailbox_read = flexcan_mailbox_read;
>
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net v1] net/mlx5e: Correctly use the namespace type when allocating pedit action
From: Roi Dayan @ 2019-02-27 14:05 UTC (permalink / raw)
To: xiangxia.m.yue@gmail.com, Saeed Mahameed, gerlitz.or@gmail.com
Cc: netdev@vger.kernel.org, Pablo Neira Ayuso
In-Reply-To: <1551184112-40686-1-git-send-email-xiangxia.m.yue@gmail.com>
On 26/02/2019 14:28, xiangxia.m.yue@gmail.com wrote:
> From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
>
> The capacity of FDB offloading and NIC offloading table are
> different, and when allocating the pedit actions, we should
> use the correct namespace type.
>
> Fixes: c500c86b0c75d ("net/mlx5e: support for two independent packet edit actions")
> Cc: Pablo Neira Ayuso <pablo@netfilter.org>
> Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 3a02b22..467ef9e 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -2635,7 +2635,7 @@ static int parse_tc_fdb_actions(struct mlx5e_priv *priv,
>
> if (hdrs[TCA_PEDIT_KEY_EX_CMD_SET].pedits ||
> hdrs[TCA_PEDIT_KEY_EX_CMD_ADD].pedits) {
> - err = alloc_tc_pedit_action(priv, MLX5_FLOW_NAMESPACE_KERNEL,
> + err = alloc_tc_pedit_action(priv, MLX5_FLOW_NAMESPACE_FDB,
> parse_attr, hdrs, extack);
> if (err)
> return err;
>
Reviewed-by: Roi Dayan <roid@mellanox.com>
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Vlad Buslov @ 2019-02-27 14:28 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <CAM_iQpVjFKwwniiFKy5ocRQ-HB=xzm-Lj6pGihFGXV6Cwj8TVQ@mail.gmail.com>
On Tue 26 Feb 2019 at 22:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>>
>> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >>
>> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
>> >> at least once is unreliable. Some classifiers set 'stop' flag on error
>> >> before calling walker callback, other classifiers used to call it with NULL
>> >> filter pointer when empty. In order to prevent further regressions, extend
>> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
>> >> tcf_walker->fn() implementation that is used to check if classifier has
>> >> filters configured.
>> >
>> >
>> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
>> > set arg->stop in fw_walk() when empty") can be reverted??
>>
>> Yes, it is safe now to revert following commits:
>>
>> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
>> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
>
> Yeah, and probably commit d66022cd1623
> ("net: sched: matchall: verify that filter is not NULL in mall_walk()").
>
> Please send a patch to revert them all.
>
> Thanks.
I think commit d66022cd1623 ("net: sched: matchall: verify that filter
is not NULL in mall_walk()") and commit 8b58d12f4ae1 ("net: sched:
cgroup: verify that filter is not NULL during walk") shouldn't be
reverted. They are still necessary to prevent tcf_chain_dump() from
dumping NULL filter pointer. It can happen when dump is initiated in
parallel with inserting first filter to unlocked classifier.
tcf_fill_node() verifies that filter pointer is not NULL, so it will not
crash, but will output tcf_proto info for second time. This might
"confuse" user-space.
What do you think?
^ permalink raw reply
* [PATCH] bpf: drop refcount if bpf_map_new_fd() fails in map_create()
From: zerons @ 2019-02-27 14:36 UTC (permalink / raw)
To: ast, daniel; +Cc: netdev, linux-kernel, Peng Sun
In bpf/syscall.c, map_create() first set map->usercnt to 1, a file descriptor is
supposed to return to userspace. When bpf_map_new_fd() fails, drop the refcount.
Signed-off-by: Peng Sun <sironhide0null@gmail.com>
---
kernel/bpf/syscall.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index cf5040f..1c4f1c4 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -558,12 +558,12 @@ static int map_create(union bpf_attr *attr)
err = bpf_map_new_fd(map, f_flags);
if (err < 0) {
/* failed to allocate fd.
- * bpf_map_put() is needed because the above
+ * bpf_map_put_with_uref() is needed because the above
* bpf_map_alloc_id() has published the map
* to the userspace and the userspace may
* have refcnt-ed it through BPF_MAP_GET_FD_BY_ID.
*/
- bpf_map_put(map);
+ bpf_map_put_with_uref(map);
return err;
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] dt-bindings: net: can: binding for CTU CAN FD open-source IP core.
From: Marc Kleine-Budde @ 2019-02-27 14:47 UTC (permalink / raw)
To: Pavel Pisa, devicetree
Cc: Wolfgang Grandegger, David S. Miller, Rob Herring, Mark Rutland,
linux-can, netdev, linux-kernel, Martin Jerabek, Ondrej Ille
In-Reply-To: <201902221420.05267.pisa@cmp.felk.cvut.cz>
[-- Attachment #1.1: Type: text/plain, Size: 5286 bytes --]
On 2/22/19 2:20 PM, Pavel Pisa wrote:
> From 3e19a7f5c33e5fb50f52c9df05bf00022e3f3dd5 Mon Sep 17 00:00:00 2001
> From: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> Date: Fri, 22 Feb 2019 14:11:11 +0100
> Subject: [PATCH] dt-bindings: net: can: binding for CTU CAN FD open-source IP
> core.
Nice! I appreciate open-source hardware!
Please send the patch via git send-email, it's a bit broken:
Applying: dt-bindings: net: can: binding for CTU CAN FD open-source IP core.
.git/rebase-apply/patch:29: trailing whitespace.
error: corrupt patch at line 30
Patch failed at 0001 dt-bindings: net: can: binding for CTU CAN FD
open-source IP core.
hint: Use 'git am --show-current-patch' to see the failed patch
> Signed-off-by: Pavel Pisa <pisa@cmp.felk.cvut.cz>
> ---
> .../devicetree/bindings/net/can/ctu,ctucanfd.txt | 102 +++++++++++++++++++++
> 1 file changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/can/ctu,ctucanfd.txt
>
> diff --git a/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.txt
> b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.txt
> new file mode 100644
> index 000000000000..6c75e5850904
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/ctu,ctucanfd.txt
> @@ -0,0 +1,102 @@
> +Memory mapped CTU CAN FD open-source IP core
> +
> +The core sources and documentation on project page
> +
> + https://gitlab.fel.cvut.cz/canbus/ctucanfd_ip_core
> + http://canbus.pages.fel.cvut.cz/ctucanfd_ip_core/Progdokum.pdf
> +
> +Integration in Xilinx Zynq SoC based system together with
> +OpenCores SJA1000 compatible controllers
> +
> + https://gitlab.fel.cvut.cz/canbus/zynq/zynq-can-sja1000-top
> +
> +Martin Jerabek's dimploma thesis with integration and testing
> +framework description
> +
> +
> https://dspace.cvut.cz/bitstream/handle/10467/80366/F3-DP-2019-Jerabek-Martin-Jerabek-thesis-2019-canfd.pdf
> +
> +Required properties:
> +
> +- compatible : should be one of "ctu,ctucanfd", "ctu,canfd-2".
> +
> +- reg = <(baseaddr) (size)> : specify mapping into physical address
> + space of the processor system.
> +
> +- interrupts : property with a value describing the interrupt source
> + required for the CTU CAN FD. For Zynq SoC system format is
> + <(is_spi) (number) (type)> where is_spi defines if it is SPI
> + (shared peripheral) interrupt, the second number is translated
> + to the vector by addition of 32 on Zynq-7000 systems and type
> + is IRQ_TYPE_LEVEL_HIGH (4) for Zynq.
> +
> +- interrupt-parent = <&interrupt-controller-phandle> :
> + is required for Zynq SoC to find map interrupt
> + to the correct controller
> +
> +- clocks: phandle of reference clock (100 MHz is appropriate
> + for FPGA implementation on Zynq-7000 system).
> +
> +Example when integrated to Zynq-7000 system DTS:
> +
> + / {
> + /* ... */
> + amba: amba {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + compatible = "simple-bus";
> +
> + CTU_CAN_FD_0: CTU_CAN_FD@43c30000 {
It's unusual to have uppercase letters here.
> + compatible = "ctu,ctucanfd";
Is ctu already registered in
Documentation/devicetree/bindings/vendor-prefixes.txt ?
> + interrupt-parent = <&intc>;
> + interrupts = <0 30 4>;
> + clocks = <&clkc 15>;
> + reg = <0x43c30000 0x10000>;
> + };
> + };
> + };
> +
> +
> +Example when used as DTS overlay on Zynq-7000 system:
> +
> +
> +// Device Tree Example: Full Reconfiguration without Bridges
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> + fragment@0 {
> + target-path = "/fpga-full";
> +
> + __overlay__ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + firmware-name = "system.bit.bin";
> + };
> + };
> +
> + fragment@1 {
> + target-path = "/amba";
> + __overlay__ {
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + CTU_CAN_FD_0: CTU_CAN_FD@43c30000 {
> + compatible = "ctu,ctucanfd";
> + interrupt-parent = <&intc>;
> + interrupts = <0 30 4>;
> + clocks = <&clkc 15>;
> + //clock-names = "can_clk";
Is this needed or not?
> + reg = <0x43c30000 0x10000>;
> + };
> + CTU_CAN_FD_1: CTU_CAN_FD@43c70000 {
> + compatible = "ctu,ctucanfd";
> + interrupt-parent = <&intc>;
> + interrupts = <0 31 4>;
> + clocks = <&clkc 15>;
> + //clock-names = "can_clk";
Is this needed or not?
> + reg = <0x43c70000 0x10000>;
> + };
> + };
> + };
> +};
>
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Industrial Linux Solutions | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH] appletalk: Fix use-after-free in atalk_proc_exit
From: Yue Haibing @ 2019-02-27 14:59 UTC (permalink / raw)
To: davem, joe, gregkh; +Cc: linux-kernel, netdev, YueHaibing
From: YueHaibing <yuehaibing@huawei.com>
KASAN report this:
BUG: KASAN: use-after-free in pde_subdir_find+0x12d/0x150 fs/proc/generic.c:71
Read of size 8 at addr ffff8881f41fe5b0 by task syz-executor.0/2806
CPU: 0 PID: 2806 Comm: syz-executor.0 Not tainted 5.0.0-rc7+ #45
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xfa/0x1ce lib/dump_stack.c:113
print_address_description+0x65/0x270 mm/kasan/report.c:187
kasan_report+0x149/0x18d mm/kasan/report.c:317
pde_subdir_find+0x12d/0x150 fs/proc/generic.c:71
remove_proc_entry+0xe8/0x420 fs/proc/generic.c:667
atalk_proc_exit+0x18/0x820 [appletalk]
atalk_exit+0xf/0x5a [appletalk]
__do_sys_delete_module kernel/module.c:1018 [inline]
__se_sys_delete_module kernel/module.c:961 [inline]
__x64_sys_delete_module+0x3dc/0x5e0 kernel/module.c:961
do_syscall_64+0x147/0x600 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x462e99
Code: f7 d8 64 89 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007fb2de6b9c58 EFLAGS: 00000246 ORIG_RAX: 00000000000000b0
RAX: ffffffffffffffda RBX: 000000000073bf00 RCX: 0000000000462e99
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00000000200001c0
RBP: 0000000000000002 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb2de6ba6bc
R13: 00000000004bccaa R14: 00000000006f6bc8 R15: 00000000ffffffff
Allocated by task 2806:
set_track mm/kasan/common.c:85 [inline]
__kasan_kmalloc.constprop.3+0xa0/0xd0 mm/kasan/common.c:496
slab_post_alloc_hook mm/slab.h:444 [inline]
slab_alloc_node mm/slub.c:2739 [inline]
slab_alloc mm/slub.c:2747 [inline]
kmem_cache_alloc+0xcf/0x250 mm/slub.c:2752
kmem_cache_zalloc include/linux/slab.h:730 [inline]
__proc_create+0x30f/0xa20 fs/proc/generic.c:408
proc_mkdir_data+0x47/0x190 fs/proc/generic.c:469
0xffffffffc10c01bb
0xffffffffc10c0166
do_one_initcall+0xfa/0x5ca init/main.c:887
do_init_module+0x204/0x5f6 kernel/module.c:3460
load_module+0x66b2/0x8570 kernel/module.c:3808
__do_sys_finit_module+0x238/0x2a0 kernel/module.c:3902
do_syscall_64+0x147/0x600 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 2806:
set_track mm/kasan/common.c:85 [inline]
__kasan_slab_free+0x130/0x180 mm/kasan/common.c:458
slab_free_hook mm/slub.c:1409 [inline]
slab_free_freelist_hook mm/slub.c:1436 [inline]
slab_free mm/slub.c:2986 [inline]
kmem_cache_free+0xa6/0x2a0 mm/slub.c:3002
pde_put+0x6e/0x80 fs/proc/generic.c:647
remove_proc_entry+0x1d3/0x420 fs/proc/generic.c:684
0xffffffffc10c031c
0xffffffffc10c0166
do_one_initcall+0xfa/0x5ca init/main.c:887
do_init_module+0x204/0x5f6 kernel/module.c:3460
load_module+0x66b2/0x8570 kernel/module.c:3808
__do_sys_finit_module+0x238/0x2a0 kernel/module.c:3902
do_syscall_64+0x147/0x600 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff8881f41fe500
which belongs to the cache proc_dir_entry of size 256
The buggy address is located 176 bytes inside of
256-byte region [ffff8881f41fe500, ffff8881f41fe600)
The buggy address belongs to the page:
page:ffffea0007d07f80 count:1 mapcount:0 mapping:ffff8881f6e69a00 index:0x0
flags: 0x2fffc0000000200(slab)
raw: 02fffc0000000200 dead000000000100 dead000000000200 ffff8881f6e69a00
raw: 0000000000000000 00000000800c000c 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8881f41fe480: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
ffff8881f41fe500: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
>ffff8881f41fe580: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8881f41fe600: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
ffff8881f41fe680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
It should check the return value of atalk_proc_init fails,
otherwise atalk_exit calls pde_subdir_find will trgger
use-after-free while unloading the module.
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/appletalk/ddp.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/appletalk/ddp.c b/net/appletalk/ddp.c
index 9b6bc5a..75075fe 100644
--- a/net/appletalk/ddp.c
+++ b/net/appletalk/ddp.c
@@ -1925,7 +1925,10 @@ static int __init atalk_init(void)
register_netdevice_notifier(&ddp_notifier);
aarp_proto_init();
- atalk_proc_init();
+ rc = atalk_proc_init();
+ if (rc)
+ goto out;
+
atalk_register_sysctl();
out:
return rc;
--
2.7.0
^ permalink raw reply related
* [PATCH -next] appletalk: use remove_proc_subtree to simplify procfs code
From: Yue Haibing @ 2019-02-27 15:00 UTC (permalink / raw)
To: davem, joe, gregkh; +Cc: linux-kernel, netdev, YueHaibing
From: YueHaibing <yuehaibing@huawei.com>
Use remove_proc_subtree to remove the whole subtree
on cleanup.Also do some cleanup.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
net/appletalk/atalk_proc.c | 56 ++++++++++++++--------------------------------
1 file changed, 17 insertions(+), 39 deletions(-)
diff --git a/net/appletalk/atalk_proc.c b/net/appletalk/atalk_proc.c
index 8006295..398b767 100644
--- a/net/appletalk/atalk_proc.c
+++ b/net/appletalk/atalk_proc.c
@@ -210,56 +210,34 @@ static const struct seq_operations atalk_seq_socket_ops = {
.show = atalk_seq_socket_show,
};
-static struct proc_dir_entry *atalk_proc_dir;
-
int __init atalk_proc_init(void)
{
- struct proc_dir_entry *p;
- int rc = -ENOMEM;
+ if (!proc_mkdir("atalk", init_net.proc_net))
+ return -ENOMEM;
- atalk_proc_dir = proc_mkdir("atalk", init_net.proc_net);
- if (!atalk_proc_dir)
+ if (!proc_create_seq("atalk/interface", 0444, init_net.proc_net,
+ &atalk_seq_interface_ops));
goto out;
- p = proc_create_seq("interface", 0444, atalk_proc_dir,
- &atalk_seq_interface_ops);
- if (!p)
- goto out_interface;
-
- p = proc_create_seq("route", 0444, atalk_proc_dir,
- &atalk_seq_route_ops);
- if (!p)
- goto out_route;
+ if (!proc_create_seq("atalk/route", 0444, init_net.proc_net,
+ &atalk_seq_route_ops));
+ goto out;
- p = proc_create_seq("socket", 0444, atalk_proc_dir,
- &atalk_seq_socket_ops);
- if (!p)
- goto out_socket;
+ if (!proc_create_seq("atalk/socket", 0444, init_net.proc_net,
+ &atalk_seq_socket_ops));
+ goto out;
- p = proc_create_seq_private("arp", 0444, atalk_proc_dir, &aarp_seq_ops,
- sizeof(struct aarp_iter_state), NULL);
- if (!p)
- goto out_arp;
+ if (!proc_create_seq_private("atalk/arp", 0444, init_net.proc_net,
+ &aarp_seq_ops,
+ sizeof(struct aarp_iter_state), NULL));
+ goto out;
- rc = 0;
out:
- return rc;
-out_arp:
- remove_proc_entry("socket", atalk_proc_dir);
-out_socket:
- remove_proc_entry("route", atalk_proc_dir);
-out_route:
- remove_proc_entry("interface", atalk_proc_dir);
-out_interface:
- remove_proc_entry("atalk", init_net.proc_net);
- goto out;
+ remove_proc_subtree("atalk", init_net.proc_net);
+ return -ENOMEM;
}
void __exit atalk_proc_exit(void)
{
- remove_proc_entry("interface", atalk_proc_dir);
- remove_proc_entry("route", atalk_proc_dir);
- remove_proc_entry("socket", atalk_proc_dir);
- remove_proc_entry("arp", atalk_proc_dir);
- remove_proc_entry("atalk", init_net.proc_net);
+ remove_proc_subtree("atalk", init_net.proc_net);
}
--
2.7.0
^ permalink raw reply related
* Re: [PATCH net 4/4] tls: Fix tls_device receive
From: Boris Pismenny @ 2019-02-27 15:23 UTC (permalink / raw)
To: Vakul Garg, Dave Watson
Cc: Aviad Yehezkel, john.fastabend@gmail.com, daniel@iogearbox.net,
netdev@vger.kernel.org, Eran Ben Elisha
In-Reply-To: <DB7PR04MB4252D936A2C713C60542A6828B740@DB7PR04MB4252.eurprd04.prod.outlook.com>
On 2/27/2019 5:08 AM, Vakul Garg wrote:
>
>
>> -----Original Message-----
>> From: Dave Watson <davejwatson@fb.com>
>> Sent: Wednesday, February 27, 2019 2:05 AM
>> To: Boris Pismenny <borisp@mellanox.com>
>> Cc: aviadye@mellanox.com; john.fastabend@gmail.com;
>> daniel@iogearbox.net; Vakul Garg <vakul.garg@nxp.com>;
>> netdev@vger.kernel.org; eranbe@mellanox.com
>> Subject: Re: [PATCH net 4/4] tls: Fix tls_device receive
>>
>> On 02/26/19 02:12 PM, Boris Pismenny wrote:
>>> Currently, the receive function fails to handle records already
>>> decrypted by the device due to the commit mentioned below.
>>>
>>> This commit advances the TLS record sequence number and prepares the
>>> context to handle the next record.
>>>
>>> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on
>>> recv")
>>> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
>>> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
>>> ---
>>> net/tls/tls_sw.c | 15 +++++++--------
>>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index
>>> f515cd7e984e..85da10182d8d 100644
>>> --- a/net/tls/tls_sw.c
>>> +++ b/net/tls/tls_sw.c
>>> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk,
>>> struct sk_buff *skb,
>>>
>>> return err;
>>> }
>>> -
>>> - rxm->full_len -= padding_length(ctx, tls_ctx, skb);
>>> -
>>> - rxm->offset += prot->prepend_size;
>>> - rxm->full_len -= prot->overhead_size;
>>> - tls_advance_record_sn(sk, &tls_ctx->rx, version);
>>> - ctx->decrypted = true;
>>> - ctx->saved_data_ready(sk);
>>> } else {
>>> *zc = false;
>>> }
>>>
>>> + rxm->full_len -= padding_length(ctx, tls_ctx, skb);
>>> + rxm->offset += prot->prepend_size;
>>> + rxm->full_len -= prot->overhead_size;
>>> + tls_advance_record_sn(sk, &tls_ctx->rx, version);
>>> + ctx->decrypted = true;
>>> + ctx->saved_data_ready(sk);
>>> +
>>> return err;
>>> }
>>
>> This breaks the tls.control_msg test:
>>
>> [ RUN ] tls.control_msg
>> tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len)
>> (18446744073709551614) == 0 (0)
>> tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len)
>> (18446744073709551614) == 0 (0)
>> tls.control_msg: Test failed at step #8
>>
>> So either control message handling needs to only call decrypt_skb_update
>> once, or we need a new flag or something to handle the device case
>
> I prefer to remove variable 'decrypted' in context.
> This is no longer required as we already have an rx_list in context for storing decrypted records.
> So for any record which we decrypted but did not return to user space
> (e.g. for the case when user used recv() and it lead to decryption of non-data record), we should
> it in rx_list.
>
IMO this is inappropriate here, because packets decrypted by tls_device
are ready to be received, and there is no reason to bounce them through
the rx_list.
^ permalink raw reply
* Re: [PATCH net 4/4] tls: Fix tls_device receive
From: Boris Pismenny @ 2019-02-27 15:26 UTC (permalink / raw)
To: Dave Watson
Cc: Aviad Yehezkel, john.fastabend@gmail.com, daniel@iogearbox.net,
vakul.garg@nxp.com, netdev@vger.kernel.org, Eran Ben Elisha
In-Reply-To: <20190226203437.c7tsjfb5ri35nn6y@iphone-a056f37cfbb1.dhcp.thefacebook.com>
On 2/26/2019 10:34 PM, Dave Watson wrote:
> On 02/26/19 02:12 PM, Boris Pismenny wrote:
>> Currently, the receive function fails to handle records already
>> decrypted by the device due to the commit mentioned below.
>>
>> This commit advances the TLS record sequence number and prepares the context
>> to handle the next record.
>>
>> Fixes: fedf201e1296 ("net: tls: Refactor control message handling on recv")
>> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
>> Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
>> ---
>> net/tls/tls_sw.c | 15 +++++++--------
>> 1 file changed, 7 insertions(+), 8 deletions(-)
>>
>> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
>> index f515cd7e984e..85da10182d8d 100644
>> --- a/net/tls/tls_sw.c
>> +++ b/net/tls/tls_sw.c
>> @@ -1481,18 +1481,17 @@ static int decrypt_skb_update(struct sock *sk, struct sk_buff *skb,
>>
>> return err;
>> }
>> -
>> - rxm->full_len -= padding_length(ctx, tls_ctx, skb);
>> -
>> - rxm->offset += prot->prepend_size;
>> - rxm->full_len -= prot->overhead_size;
>> - tls_advance_record_sn(sk, &tls_ctx->rx, version);
>> - ctx->decrypted = true;
>> - ctx->saved_data_ready(sk);
>> } else {
>> *zc = false;
>> }
>>
>> + rxm->full_len -= padding_length(ctx, tls_ctx, skb);
>> + rxm->offset += prot->prepend_size;
>> + rxm->full_len -= prot->overhead_size;
>> + tls_advance_record_sn(sk, &tls_ctx->rx, version);
>> + ctx->decrypted = true;
>> + ctx->saved_data_ready(sk);
>> +
>> return err;
>> }
>
> This breaks the tls.control_msg test:
>
> [ RUN ] tls.control_msg
> tls.c:764:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0)
> tls.c:777:tls.control_msg:Expected memcmp(buf, test_str, send_len) (18446744073709551614) == 0 (0)
> tls.control_msg: Test failed at step #8
>
> So either control message handling needs to only call
> decrypt_skb_update once, or we need a new flag or something to handle
> the device case
>
Thanks for raising this, I'm not used to the kselftests yet.
I've refactored the code here to get this working.
Will send V2 soon.
^ permalink raw reply
* [PATCH v2 net 2/4] tls: Fix write space handling
From: Boris Pismenny @ 2019-02-27 15:38 UTC (permalink / raw)
To: aviadye, davejwatson, john.fastabend, daniel, vakul.garg, netdev
Cc: eranbe, borisp
In-Reply-To: <20190227153806.17080-1-borisp@mellanox.com>
TLS device cannot use the sw context. This patch returns the original
tls device write space handler and moves the sw/device specific portions
to the relevant files.
Also, we remove the write_space call for the tls_sw flow, because it
handles partial records in its delayed tx work handler.
Fixes: a42055e8d2c3 ("net/tls: Add support for async encryption of records for performance")
Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Reviewed-by: Eran Ben Elisha <eranbe@mellanox.com>
---
include/net/tls.h | 3 +++
net/tls/tls_device.c | 17 +++++++++++++++++
net/tls/tls_main.c | 15 ++++++---------
net/tls/tls_sw.c | 13 +++++++++++++
4 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/include/net/tls.h b/include/net/tls.h
index a528a082da73..a5a938583295 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -519,6 +519,9 @@ static inline bool tls_sw_has_ctx_tx(const struct sock *sk)
return !!tls_sw_ctx_tx(ctx);
}
+void tls_sw_write_space(struct sock *sk, struct tls_context *ctx);
+void tls_device_write_space(struct sock *sk, struct tls_context *ctx);
+
static inline struct tls_offload_context_rx *
tls_offload_ctx_rx(const struct tls_context *tls_ctx)
{
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 3e5e8e021a87..4a1da837a733 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -546,6 +546,23 @@ static int tls_device_push_pending_record(struct sock *sk, int flags)
return tls_push_data(sk, &msg_iter, 0, flags, TLS_RECORD_TYPE_DATA);
}
+void tls_device_write_space(struct sock *sk, struct tls_context *ctx)
+{
+ int rc = 0;
+
+ if (!sk->sk_write_pending && tls_is_partially_sent_record(ctx)) {
+ gfp_t sk_allocation = sk->sk_allocation;
+
+ sk->sk_allocation = GFP_ATOMIC;
+ rc = tls_push_partial_record(sk, ctx,
+ MSG_DONTWAIT | MSG_NOSIGNAL);
+ sk->sk_allocation = sk_allocation;
+ }
+
+ if (!rc)
+ ctx->sk_write_space(sk);
+}
+
void handle_device_resync(struct sock *sk, u32 seq, u64 rcd_sn)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 7e05af75536d..17e8667917aa 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -212,7 +212,6 @@ int tls_push_partial_record(struct sock *sk, struct tls_context *ctx,
static void tls_write_space(struct sock *sk)
{
struct tls_context *ctx = tls_get_ctx(sk);
- struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
/* If in_tcp_sendpages call lower protocol write space handler
* to ensure we wake up any waiting operations there. For example
@@ -223,14 +222,12 @@ static void tls_write_space(struct sock *sk)
return;
}
- /* Schedule the transmission if tx list is ready */
- if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
- /* Schedule the transmission */
- if (!test_and_set_bit(BIT_TX_SCHEDULED, &tx_ctx->tx_bitmask))
- schedule_delayed_work(&tx_ctx->tx_work.work, 0);
- }
-
- ctx->sk_write_space(sk);
+#ifdef CONFIG_TLS_DEVICE
+ if (ctx->tx_conf == TLS_HW)
+ tls_device_write_space(sk, ctx);
+ else
+#endif
+ tls_sw_write_space(sk, ctx);
}
static void tls_ctx_free(struct tls_context *ctx)
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 1cc830582fa8..917caacd4d31 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2126,6 +2126,19 @@ static void tx_work_handler(struct work_struct *work)
release_sock(sk);
}
+void tls_sw_write_space(struct sock *sk, struct tls_context *ctx)
+{
+ struct tls_sw_context_tx *tx_ctx = tls_sw_ctx_tx(ctx);
+
+ /* Schedule the transmission if tx list is ready */
+ if (is_tx_ready(tx_ctx) && !sk->sk_write_pending) {
+ /* Schedule the transmission */
+ if (!test_and_set_bit(BIT_TX_SCHEDULED,
+ &tx_ctx->tx_bitmask))
+ schedule_delayed_work(&tx_ctx->tx_work.work, 0);
+ }
+}
+
int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
{
struct tls_context *tls_ctx = tls_get_ctx(sk);
--
2.12.2
^ 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