* [PATCH bpf-next 03/12] nfp: bpf: copy eBPF subprograms information from kernel verifier
From: Quentin Monnet @ 2018-10-07 11:56 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev, oss-drivers, Quentin Monnet
In-Reply-To: <1538913418-16039-1-git-send-email-quentin.monnet@netronome.com>
In order to support BPF-to-BPF calls in offloaded programs, the nfp
driver must collect information about the distinct subprograms: namely,
the number of subprograms composing the complete program and the stack
depth of those subprograms. The latter in particular is non-trivial to
collect, so we copy those elements from the kernel verifier via the
newly added post-verification hook. The struct nfp_prog is extended to
store this information. Stack depths are stored in an array of dedicated
structs.
Subprogram start indexes are not collected. Instead, meta instructions
associated to the start of a subprogram will be marked with a flag in a
later patch.
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/main.h | 12 ++++++++++++
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 2 ++
drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 15 +++++++++++++++
3 files changed, 29 insertions(+)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 7050535383b8..7f6e850e42da 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -424,6 +424,14 @@ static inline bool is_mbpf_div(const struct nfp_insn_meta *meta)
}
/**
+ * struct nfp_bpf_subprog_info - nfp BPF sub-program (a.k.a. function) info
+ * @stack_depth: maximum stack depth used by this sub-program
+ */
+struct nfp_bpf_subprog_info {
+ u16 stack_depth;
+};
+
+/**
* struct nfp_prog - nfp BPF program
* @bpf: backpointer to the bpf app priv structure
* @prog: machine code
@@ -439,7 +447,9 @@ static inline bool is_mbpf_div(const struct nfp_insn_meta *meta)
* @stack_frame_depth: max stack depth for current frame
* @adjust_head_location: if program has single adjust head call - the insn no.
* @map_records_cnt: the number of map pointers recorded for this prog
+ * @subprog_cnt: number of sub-programs, including main function
* @map_records: the map record pointers from bpf->maps_neutral
+ * @subprog: pointer to an array of objects holding info about sub-programs
* @insns: list of BPF instruction wrappers (struct nfp_insn_meta)
*/
struct nfp_prog {
@@ -464,7 +474,9 @@ struct nfp_prog {
unsigned int adjust_head_location;
unsigned int map_records_cnt;
+ unsigned int subprog_cnt;
struct nfp_bpf_neutral_map **map_records;
+ struct nfp_bpf_subprog_info *subprog;
struct list_head insns;
};
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index c9519bb00f8a..b683b03efd22 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -208,6 +208,8 @@ static void nfp_prog_free(struct nfp_prog *nfp_prog)
{
struct nfp_insn_meta *meta, *tmp;
+ kfree(nfp_prog->subprog);
+
list_for_each_entry_safe(meta, tmp, &nfp_prog->insns, l) {
list_del(&meta->l);
kfree(meta);
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index e470489021e3..9ef74bc1ec1d 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -642,6 +642,21 @@ nfp_verify_insn(struct bpf_verifier_env *env, int insn_idx, int prev_insn_idx)
static int nfp_bpf_finalize(struct bpf_verifier_env *env)
{
+ struct bpf_subprog_info *info;
+ struct nfp_prog *nfp_prog;
+ int i;
+
+ nfp_prog = env->prog->aux->offload->dev_priv;
+ nfp_prog->subprog_cnt = env->subprog_cnt;
+ nfp_prog->subprog = kcalloc(nfp_prog->subprog_cnt,
+ sizeof(nfp_prog->subprog[0]), GFP_KERNEL);
+ if (!nfp_prog->subprog)
+ return -ENOMEM;
+
+ info = env->subprog_info;
+ for (i = 0; i < nfp_prog->subprog_cnt; i++)
+ nfp_prog->subprog[i].stack_depth = info[i].stack_depth;
+
return 0;
}
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next 02/12] nfp: bpf: rename nfp_prog->stack_depth as nfp_prog->stack_frame_depth
From: Quentin Monnet @ 2018-10-07 11:56 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev, oss-drivers, Quentin Monnet
In-Reply-To: <1538913418-16039-1-git-send-email-quentin.monnet@netronome.com>
In preparation for support for BPF to BPF calls in offloaded programs,
rename the "stack_depth" field of the struct nfp_prog as
"stack_frame_depth". This is to make it clear that the field refers to
the maximum size of the current stack frame (as opposed to the maximum
size of the whole stack memory).
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/jit.c | 10 +++++-----
drivers/net/ethernet/netronome/nfp/bpf/main.h | 4 ++--
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 2 +-
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/jit.c b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
index eff57f7d056a..98a94ca36bfa 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/jit.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/jit.c
@@ -1137,7 +1137,7 @@ mem_op_stack(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta,
unsigned int size, unsigned int ptr_off, u8 gpr, u8 ptr_gpr,
bool clr_gpr, lmem_step step)
{
- s32 off = nfp_prog->stack_depth + meta->insn.off + ptr_off;
+ s32 off = nfp_prog->stack_frame_depth + meta->insn.off + ptr_off;
bool first = true, last;
bool needs_inc = false;
swreg stack_off_reg;
@@ -1695,7 +1695,7 @@ map_call_stack_common(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
s64 lm_off;
/* We only have to reload LM0 if the key is not at start of stack */
- lm_off = nfp_prog->stack_depth;
+ lm_off = nfp_prog->stack_frame_depth;
lm_off += meta->arg2.reg.var_off.value + meta->arg2.reg.off;
load_lm_ptr = meta->arg2.var_off || lm_off;
@@ -1808,10 +1808,10 @@ static int mov_reg64(struct nfp_prog *nfp_prog, struct nfp_insn_meta *meta)
swreg stack_depth_reg;
stack_depth_reg = ur_load_imm_any(nfp_prog,
- nfp_prog->stack_depth,
+ nfp_prog->stack_frame_depth,
stack_imm(nfp_prog));
- emit_alu(nfp_prog, reg_both(dst),
- stack_reg(nfp_prog), ALU_OP_ADD, stack_depth_reg);
+ emit_alu(nfp_prog, reg_both(dst), stack_reg(nfp_prog),
+ ALU_OP_ADD, stack_depth_reg);
wrp_immed(nfp_prog, reg_both(dst + 1), 0);
} else {
wrp_reg_mov(nfp_prog, dst, src);
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/main.h b/drivers/net/ethernet/netronome/nfp/bpf/main.h
index 792ebc4081a3..7050535383b8 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/main.h
+++ b/drivers/net/ethernet/netronome/nfp/bpf/main.h
@@ -436,7 +436,7 @@ static inline bool is_mbpf_div(const struct nfp_insn_meta *meta)
* @tgt_abort: jump target for abort (e.g. access outside of packet buffer)
* @n_translated: number of successfully translated instructions (for errors)
* @error: error code if something went wrong
- * @stack_depth: max stack depth from the verifier
+ * @stack_frame_depth: max stack depth for current frame
* @adjust_head_location: if program has single adjust head call - the insn no.
* @map_records_cnt: the number of map pointers recorded for this prog
* @map_records: the map record pointers from bpf->maps_neutral
@@ -460,7 +460,7 @@ struct nfp_prog {
unsigned int n_translated;
int error;
- unsigned int stack_depth;
+ unsigned int stack_frame_depth;
unsigned int adjust_head_location;
unsigned int map_records_cnt;
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/offload.c b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
index 1ccd6371a15b..c9519bb00f8a 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/offload.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/offload.c
@@ -260,7 +260,7 @@ static int nfp_bpf_translate(struct nfp_net *nn, struct bpf_prog *prog)
prog->aux->stack_depth, stack_size);
return -EOPNOTSUPP;
}
- nfp_prog->stack_depth = round_up(prog->aux->stack_depth, 4);
+ nfp_prog->stack_frame_depth = round_up(prog->aux->stack_depth, 4);
max_instr = nn_readw(nn, NFP_NET_CFG_BPF_MAX_LEN);
nfp_prog->__prog_alloc_len = max_instr * sizeof(u64);
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next 01/12] bpf: add verifier callback to get stack usage info for offloaded progs
From: Quentin Monnet @ 2018-10-07 11:56 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev, oss-drivers, Quentin Monnet
In-Reply-To: <1538913418-16039-1-git-send-email-quentin.monnet@netronome.com>
In preparation for BPF-to-BPF calls in offloaded programs, add a new
function attribute to the struct bpf_prog_offload_ops so that drivers
supporting eBPF offload can hook at the end of program verification, and
potentially extract information collected by the verifier.
Implement a minimal callback (returning 0) in the drivers providing the
structs, namely netdevsim and nfp.
This will be useful in the nfp driver, in later commits, to extract the
number of subprograms as well as the stack depth for those subprograms.
Signed-off-by: Quentin Monnet <quentin.monnet@netronome.com>
Reviewed-by: Jiong Wang <jiong.wang@netronome.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 8 +++++++-
drivers/net/netdevsim/bpf.c | 8 +++++++-
include/linux/bpf.h | 1 +
include/linux/bpf_verifier.h | 1 +
kernel/bpf/offload.c | 18 ++++++++++++++++++
kernel/bpf/verifier.c | 3 +++
6 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
index a6e9248669e1..e470489021e3 100644
--- a/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
+++ b/drivers/net/ethernet/netronome/nfp/bpf/verifier.c
@@ -640,6 +640,12 @@ nfp_verify_insn(struct bpf_verifier_env *env, int insn_idx, int prev_insn_idx)
return 0;
}
+static int nfp_bpf_finalize(struct bpf_verifier_env *env)
+{
+ return 0;
+}
+
const struct bpf_prog_offload_ops nfp_bpf_analyzer_ops = {
- .insn_hook = nfp_verify_insn,
+ .insn_hook = nfp_verify_insn,
+ .finalize = nfp_bpf_finalize,
};
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 81444208b216..cb3518474f0e 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -86,8 +86,14 @@ nsim_bpf_verify_insn(struct bpf_verifier_env *env, int insn_idx, int prev_insn)
return 0;
}
+static int nsim_bpf_finalize(struct bpf_verifier_env *env)
+{
+ return 0;
+}
+
static const struct bpf_prog_offload_ops nsim_bpf_analyzer_ops = {
- .insn_hook = nsim_bpf_verify_insn,
+ .insn_hook = nsim_bpf_verify_insn,
+ .finalize = nsim_bpf_finalize,
};
static bool nsim_xdp_offload_active(struct netdevsim *ns)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 027697b6a22f..9b558713447f 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -263,6 +263,7 @@ struct bpf_verifier_ops {
struct bpf_prog_offload_ops {
int (*insn_hook)(struct bpf_verifier_env *env,
int insn_idx, int prev_insn_idx);
+ int (*finalize)(struct bpf_verifier_env *env);
};
struct bpf_prog_offload {
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 7b6fd2ab3263..9e8056ec20fa 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -245,5 +245,6 @@ static inline struct bpf_reg_state *cur_regs(struct bpf_verifier_env *env)
int bpf_prog_offload_verifier_prep(struct bpf_verifier_env *env);
int bpf_prog_offload_verify_insn(struct bpf_verifier_env *env,
int insn_idx, int prev_insn_idx);
+int bpf_prog_offload_finalize(struct bpf_verifier_env *env);
#endif /* _LINUX_BPF_VERIFIER_H */
diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
index 177a52436394..8e93c47f0779 100644
--- a/kernel/bpf/offload.c
+++ b/kernel/bpf/offload.c
@@ -172,6 +172,24 @@ int bpf_prog_offload_verify_insn(struct bpf_verifier_env *env,
return ret;
}
+int bpf_prog_offload_finalize(struct bpf_verifier_env *env)
+{
+ struct bpf_prog_offload *offload;
+ int ret = -ENODEV;
+
+ down_read(&bpf_devs_lock);
+ offload = env->prog->aux->offload;
+ if (offload) {
+ if (offload->dev_ops->finalize)
+ ret = offload->dev_ops->finalize(env);
+ else
+ ret = 0;
+ }
+ up_read(&bpf_devs_lock);
+
+ return ret;
+}
+
static void __bpf_prog_offload_destroy(struct bpf_prog *prog)
{
struct bpf_prog_offload *offload = prog->aux->offload;
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 73c81bef6ae8..a0454cb299ba 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -6309,6 +6309,9 @@ int bpf_check(struct bpf_prog **prog, union bpf_attr *attr)
env->cur_state = NULL;
}
+ if (ret == 0 && bpf_prog_is_dev_bound(env->prog->aux))
+ ret = bpf_prog_offload_finalize(env);
+
skip_full_check:
while (!pop_stack(env, NULL, NULL));
free_states(env);
--
2.7.4
^ permalink raw reply related
* [PATCH bpf-next 00/12] nfp: bpf: add support for BPF-to-BPF function calls
From: Quentin Monnet @ 2018-10-07 11:56 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov; +Cc: netdev, oss-drivers, Quentin Monnet
This patch series adds support for hardware offload of programs containing
BPF-to-BPF function calls. First, a new callback is added to the kernel
verifier, to collect information after the main part of the verification
has been performed. Then support for BPF-to-BPF calls is incrementally
added to the nfp driver, before offloading programs containing such calls
is eventually allowed by lifting the restriction in the kernel verifier, in
the last patch. Please refer to individual patches for details.
Many thanks to Jiong and Jakub for their precious help and contribution on
the main patches for the JIT-compiler, and everything related to stack
accesses.
Quentin Monnet (12):
bpf: add verifier callback to get stack usage info for offloaded progs
nfp: bpf: rename nfp_prog->stack_depth as nfp_prog->stack_frame_depth
nfp: bpf: copy eBPF subprograms information from kernel verifier
nfp: bpf: ignore helper-related checks for BPF calls in nfp verifier
nfp: bpf: account for BPF-to-BPF calls when preparing nfp JIT
nfp: bpf: add main logics for BPF-to-BPF calls support in nfp driver
nfp: bpf: account for additional stack usage when checking stack limit
nfp: bpf: update fixup function for BPF-to-BPF calls support
nfp: bpf: fix return address from register-saving subroutine to callee
nfp: bpf: optimise save/restore for R6~R9 based on register usage
nfp: bpf: support pointers to other stack frames for BPF-to-BPF calls
bpf: allow offload of programs with BPF-to-BPF function calls
drivers/net/ethernet/netronome/nfp/bpf/jit.c | 381 ++++++++++++++++++++--
drivers/net/ethernet/netronome/nfp/bpf/main.h | 52 ++-
drivers/net/ethernet/netronome/nfp/bpf/offload.c | 11 +-
drivers/net/ethernet/netronome/nfp/bpf/verifier.c | 141 +++++++-
drivers/net/ethernet/netronome/nfp/nfp_asm.h | 9 +
drivers/net/netdevsim/bpf.c | 8 +-
include/linux/bpf.h | 1 +
include/linux/bpf_verifier.h | 1 +
kernel/bpf/offload.c | 18 +
kernel/bpf/verifier.c | 13 +-
10 files changed, 589 insertions(+), 46 deletions(-)
--
2.7.4
^ permalink raw reply
* [PATCH net 1/1] net: sched: cls_u32: fix hnode refcounting
From: Jamal Hadi Salim @ 2018-10-07 11:40 UTC (permalink / raw)
To: davem; +Cc: xiyou.wangcong, jiri, netdev, viro, Jamal Hadi Salim
From: Al Viro <viro@zeniv.linux.org.uk>
cls_u32.c misuses refcounts for struct tc_u_hnode - it counts references
via ->hlist and via ->tp_root together. u32_destroy() drops the former
and, in case when there had been links, leaves the sucker on the list.
As the result, there's nothing to protect it from getting freed once links
are dropped.
That also makes the "is it busy" check incapable of catching the root
hnode - it *is* busy (there's a reference from tp), but we don't see it as
something separate. "Is it our root?" check partially covers that, but
the problem exists for others' roots as well.
AFAICS, the minimal fix preserving the existing behaviour (where it doesn't
include oopsen, that is) would be this:
* count tp->root and tp_c->hlist as separate references. I.e.
have u32_init() set refcount to 2, not 1.
* in u32_destroy() we always drop the former;
in u32_destroy_hnode() - the latter.
That way we have *all* references contributing to refcount. List
removal happens in u32_destroy_hnode() (called only when ->refcnt is 1)
an in u32_destroy() in case of tc_u_common going away, along with
everything reachable from it. IOW, that way we know that
u32_destroy_key() won't free something still on the list (or pointed to by
someone's ->root).
Reproducer:
tc qdisc add dev eth0 ingress
tc filter add dev eth0 parent ffff: protocol ip prio 100 handle 1: \
u32 divisor 1
tc filter add dev eth0 parent ffff: protocol ip prio 200 handle 2: \
u32 divisor 1
tc filter add dev eth0 parent ffff: protocol ip prio 100 \
handle 1:0:11 u32 ht 1: link 801: offset at 0 mask 0f00 shift 6 \
plus 0 eat match ip protocol 6 ff
tc filter delete dev eth0 parent ffff: protocol ip prio 200
tc filter change dev eth0 parent ffff: protocol ip prio 100 \
handle 1:0:11 u32 ht 1: link 0: offset at 0 mask 0f00 shift 6 plus 0 \
eat match ip protocol 6 ff
tc filter delete dev eth0 parent ffff: protocol ip prio 100
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
net/sched/cls_u32.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index f218ccf1e2d9..b2c3406a2cf2 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -398,6 +398,7 @@ static int u32_init(struct tcf_proto *tp)
rcu_assign_pointer(tp_c->hlist, root_ht);
root_ht->tp_c = tp_c;
+ root_ht->refcnt++;
rcu_assign_pointer(tp->root, root_ht);
tp->data = tp_c;
return 0;
@@ -610,7 +611,7 @@ static int u32_destroy_hnode(struct tcf_proto *tp, struct tc_u_hnode *ht,
struct tc_u_hnode __rcu **hn;
struct tc_u_hnode *phn;
- WARN_ON(ht->refcnt);
+ WARN_ON(--ht->refcnt);
u32_clear_hnode(tp, ht, extack);
@@ -649,7 +650,7 @@ static void u32_destroy(struct tcf_proto *tp, struct netlink_ext_ack *extack)
WARN_ON(root_ht == NULL);
- if (root_ht && --root_ht->refcnt == 0)
+ if (root_ht && --root_ht->refcnt == 1)
u32_destroy_hnode(tp, root_ht, extack);
if (--tp_c->refcnt == 0) {
@@ -698,7 +699,6 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
}
if (ht->refcnt == 1) {
- ht->refcnt--;
u32_destroy_hnode(tp, ht, extack);
} else {
NL_SET_ERR_MSG_MOD(extack, "Can not delete in-use filter");
@@ -708,11 +708,11 @@ static int u32_delete(struct tcf_proto *tp, void *arg, bool *last,
out:
*last = true;
if (root_ht) {
- if (root_ht->refcnt > 1) {
+ if (root_ht->refcnt > 2) {
*last = false;
goto ret;
}
- if (root_ht->refcnt == 1) {
+ if (root_ht->refcnt == 2) {
if (!ht_empty(root_ht)) {
*last = false;
goto ret;
--
2.11.0
^ permalink raw reply related
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-10-07 18:42 UTC (permalink / raw)
To: Lukas Wunner
Cc: Jason A. Donenfeld, Ji??í Pírko, LKML, Netdev,
David Miller, Greg Kroah-Hartman, Dan Carpenter
In-Reply-To: <20181007181444.wohudtigexv56b77@wunner.de>
> Apart from Dan's clarity argument, what if you need to insert another
> step to create the interface, thereby necessitating another step in
> the error path? Are you going to call it 4a, 4b, ...? Because you
> don't want to inflate that future patch by renaming every single
> label.
Hi Lukas
You beat me to it.
Jason, the things to understand is, Wireguard is not finished. It has
only just started. The code will develop further, changes will be
made. We need the code to be easy to make changes to. As Lukas just
pointed out, this number construct is hard to change in a minimal way,
making a patch small, and obviously correct.
Andrew
^ permalink raw reply
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Lukas Wunner @ 2018-10-07 18:14 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Ji??í Pírko, LKML, Netdev, David Miller,
Greg Kroah-Hartman, Dan Carpenter
In-Reply-To: <CAHmME9pKvUOaAHhe7gb3yH4jdkdTtLv+gUqdnh3bJZcN776UWQ@mail.gmail.com>
On Sun, Oct 07, 2018 at 07:26:47PM +0200, Jason A. Donenfeld wrote:
> On Sat, Oct 6, 2018 at 9:03 AM Jiri Pirko <jiri@resnulli.us> wrote:
> > >+ wg->incoming_handshakes_worker =
> > >+ wg_packet_alloc_percpu_multicore_worker(
> > >+ wg_packet_handshake_receive_worker, wg);
> > >+ if (!wg->incoming_handshakes_worker)
> > >+ goto error_2;
> >
> >
> > Please consider renaming the label to "what went wrong". In this case,
> > it would be "err_alloc_worker".
Dan Carpenter, who has probably become the world expert on error paths
in the kernel due to his work on smatch, recommends naming the labels
not "what went wrong" but rather what the error path is going to do,
in this case "err_free_incoming_handshakes_worker" (abbreviate to
"err_free_incoming" or some such):
https://lkml.org/lkml/2016/8/22/374
> > >+ pr_debug("%s: Interface created\n", dev->name);
> > >+ return ret;
> > >+
> > >+error_9:
> > >+ wg_ratelimiter_uninit();
> > >+error_8:
> > >+ wg_packet_queue_free(&wg->decrypt_queue, true);
> > >+error_7:
> > >+ wg_packet_queue_free(&wg->encrypt_queue, true);
> > >+error_6:
> > >+ destroy_workqueue(wg->packet_crypt_wq);
> > >+error_5:
> > >+ destroy_workqueue(wg->handshake_send_wq);
> > >+error_4:
> > >+ destroy_workqueue(wg->handshake_receive_wq);
> > >+error_3:
> > >+ free_percpu(wg->incoming_handshakes_worker);
> > >+error_2:
> > >+ free_percpu(dev->tstats);
> > >+error_1:
> > >+ return ret;
> > >+}
>
> I'll change away from using error_9,8,7,6,5,4,3,2,1 because of your
> suggestion -- and because it's the norm in the kernel to use real
> names. But, I would be interested in your opinion on the numerical
> errors' reasoning for existing in the first place. The idea was that
> with so many different failure cases that need to cascade in the
> correct order, it's much easier to visually inspect that it's been
> done right by observing up top 1,2,3,4,5,6,7,8,9 and at the bottom
> 9,8,7,6,5,4,3,2,1, rather than having to store in my brain's limited
> stack space what each name pertains to and keep track of the ordering
> and such. In light of that, do you still think that following the
> convention of textual error labels is a good match here? Again, I'm
> changing this for v8, but I am nonetheless curious about what you
> think.
Apart from Dan's clarity argument, what if you need to insert another
step to create the interface, thereby necessitating another step in
the error path? Are you going to call it 4a, 4b, ...? Because you
don't want to inflate that future patch by renaming every single
label.
Thanks,
Lukas
^ permalink raw reply
* Re: [PATCH net-next 17/20] net/fib_rules: Update fib_nl_dumprule for strict data checking
From: Christian Brauner @ 2018-10-07 10:55 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-18-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:52PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update fib_nl_dumprule for strict data checking. If the flag is set,
> the dump request is expected to have fib_rule_hdr struct as the header.
> All elements of the struct are expected to be 0 and no attributes can
> be appended.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> net/core/fib_rules.c | 36 +++++++++++++++++++++++++++++++++++-
> 1 file changed, 35 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/fib_rules.c b/net/core/fib_rules.c
> index 0ff3953f64aa..e3cf50728d0a 100644
> --- a/net/core/fib_rules.c
> +++ b/net/core/fib_rules.c
> @@ -1063,13 +1063,47 @@ static int dump_rules(struct sk_buff *skb, struct netlink_callback *cb,
> return err;
> }
>
> +static int fib_valid_dumprule(const struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct fib_rule_hdr *frh;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*frh))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + frh = nlmsg_data(nlh);
> + if (frh->dst_len || frh->src_len || frh->tos || frh->table ||
> + frh->res1 || frh->res2 || frh->action || frh->flags) {
> + NL_SET_ERR_MSG(extack,
> + "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*frh))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int fib_nl_dumprule(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> struct fib_rules_ops *ops;
> int idx = 0, family;
>
> - family = rtnl_msg_family(cb->nlh);
> + if (cb->strict_check) {
> + int err = fib_valid_dumprule(nlh, cb->extack);
> +
> + if (err)
> + return err;
> + }
> +
> + family = rtnl_msg_family(nlh);
> if (family != AF_UNSPEC) {
> /* Protocol specific dump request */
> ops = lookup_rules_ops(net, family);
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 18/20] net/ipv6: Update ip6addrlbl_dump for strict data checking
From: Christian Brauner @ 2018-10-07 10:54 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-19-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:53PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update ip6addrlbl_dump for strict data checking. If the flag is set,
> the dump request is expected to have an ifaddrlblmsg struct as the
> header. All elements of the struct are expected to be 0 and no
> attributes can be appended.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> net/ipv6/addrlabel.c | 35 ++++++++++++++++++++++++++++++++++-
> 1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv6/addrlabel.c b/net/ipv6/addrlabel.c
> index 1d6ced37ad71..10556049cc44 100644
> --- a/net/ipv6/addrlabel.c
> +++ b/net/ipv6/addrlabel.c
> @@ -458,20 +458,53 @@ static int ip6addrlbl_fill(struct sk_buff *skb,
> return 0;
> }
>
> +static int ip6addrlbl_valid_dump_req(const struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct ifaddrlblmsg *ifal;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifal))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + ifal = nlmsg_data(nlh);
> + if (ifal->__ifal_reserved || ifal->ifal_prefixlen ||
> + ifal->ifal_flags || ifal->ifal_index || ifal->ifal_seq) {
> + NL_SET_ERR_MSG(extack,
> + "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*ifal))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int ip6addrlbl_dump(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> struct ip6addrlbl_entry *p;
> int idx = 0, s_idx = cb->args[0];
> int err;
>
> + if (cb->strict_check) {
> + err = ip6addrlbl_valid_dump_req(nlh, cb->extack);
> + if (err)
> + return err;
> + }
> +
> rcu_read_lock();
> hlist_for_each_entry_rcu(p, &net->ipv6.ip6addrlbl_table.head, list) {
> if (idx >= s_idx) {
> err = ip6addrlbl_fill(skb, p,
> net->ipv6.ip6addrlbl_table.seq,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWADDRLABEL,
> NLM_F_MULTI);
> if (err < 0)
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 19/20] net: Update netconf dump handlers for strict data checking
From: Christian Brauner @ 2018-10-07 10:53 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-20-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:54PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update inet_netconf_dump_devconf, inet6_netconf_dump_devconf, and
> mpls_netconf_dump_devconf for strict data checking. If the flag is set,
> the dump request is expected to have an netconfmsg struct as the header.
> The struct only has the family member and no attributes can be appended.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> net/ipv4/devinet.c | 22 +++++++++++++++++++---
> net/ipv6/addrconf.c | 22 +++++++++++++++++++---
> net/mpls/af_mpls.c | 18 +++++++++++++++++-
> 3 files changed, 55 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/devinet.c b/net/ipv4/devinet.c
> index af968d4fe4fc..595706d6b672 100644
> --- a/net/ipv4/devinet.c
> +++ b/net/ipv4/devinet.c
> @@ -2069,6 +2069,7 @@ static int inet_netconf_get_devconf(struct sk_buff *in_skb,
> static int inet_netconf_dump_devconf(struct sk_buff *skb,
> struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> int h, s_h;
> int idx, s_idx;
> @@ -2076,6 +2077,21 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,
> struct in_device *in_dev;
> struct hlist_head *head;
>
> + if (cb->strict_check) {
> + struct netlink_ext_ack *extack = cb->extack;
> + struct netconfmsg *ncm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ncm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*ncm))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
Hm, I think this could just be one branch with !=
But if you've done this to report back a more meaningful error message
to userspace, fine. :)
> + }
> +
> s_h = cb->args[0];
> s_idx = idx = cb->args[1];
>
> @@ -2095,7 +2111,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,
> if (inet_netconf_fill_devconf(skb, dev->ifindex,
> &in_dev->cnf,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF,
> NLM_F_MULTI,
> NETCONFA_ALL) < 0) {
> @@ -2112,7 +2128,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,
> if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
> net->ipv4.devconf_all,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF, NLM_F_MULTI,
> NETCONFA_ALL) < 0)
> goto done;
> @@ -2123,7 +2139,7 @@ static int inet_netconf_dump_devconf(struct sk_buff *skb,
> if (inet_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
> net->ipv4.devconf_dflt,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF, NLM_F_MULTI,
> NETCONFA_ALL) < 0)
> goto done;
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 693199a29426..9dfe6c2106c1 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -666,6 +666,7 @@ static int inet6_netconf_get_devconf(struct sk_buff *in_skb,
> static int inet6_netconf_dump_devconf(struct sk_buff *skb,
> struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> int h, s_h;
> int idx, s_idx;
> @@ -673,6 +674,21 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,
> struct inet6_dev *idev;
> struct hlist_head *head;
>
> + if (cb->strict_check) {
> + struct netlink_ext_ack *extack = cb->extack;
> + struct netconfmsg *ncm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ncm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*ncm))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> + }
> +
> s_h = cb->args[0];
> s_idx = idx = cb->args[1];
>
> @@ -692,7 +708,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,
> if (inet6_netconf_fill_devconf(skb, dev->ifindex,
> &idev->cnf,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF,
> NLM_F_MULTI,
> NETCONFA_ALL) < 0) {
> @@ -709,7 +725,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,
> if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_ALL,
> net->ipv6.devconf_all,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF, NLM_F_MULTI,
> NETCONFA_ALL) < 0)
> goto done;
> @@ -720,7 +736,7 @@ static int inet6_netconf_dump_devconf(struct sk_buff *skb,
> if (inet6_netconf_fill_devconf(skb, NETCONFA_IFINDEX_DEFAULT,
> net->ipv6.devconf_dflt,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF, NLM_F_MULTI,
> NETCONFA_ALL) < 0)
> goto done;
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 3e33934751b4..b80b00b55bdf 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -1263,6 +1263,7 @@ static int mpls_netconf_get_devconf(struct sk_buff *in_skb,
> static int mpls_netconf_dump_devconf(struct sk_buff *skb,
> struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> struct hlist_head *head;
> struct net_device *dev;
> @@ -1270,6 +1271,21 @@ static int mpls_netconf_dump_devconf(struct sk_buff *skb,
> int idx, s_idx;
> int h, s_h;
>
> + if (cb->strict_check) {
> + struct netlink_ext_ack *extack = cb->extack;
> + struct netconfmsg *ncm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ncm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*ncm))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> + }
> +
> s_h = cb->args[0];
> s_idx = idx = cb->args[1];
>
> @@ -1286,7 +1302,7 @@ static int mpls_netconf_dump_devconf(struct sk_buff *skb,
> goto cont;
> if (mpls_netconf_fill_devconf(skb, mdev,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNETCONF,
> NLM_F_MULTI,
> NETCONFA_ALL) < 0) {
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 15/20] net/neighbor: Update neightbl_dump_info for strict data checking
From: Christian Brauner @ 2018-10-07 10:48 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-16-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:50PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update neightbl_dump_info for strict data checking. If the flag is set,
> the dump request is expected to have an ndtmsg struct as the header.
> All elements of the struct are expected to be 0 and no attributes can
> be appended.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/core/neighbour.c | 38 +++++++++++++++++++++++++++++++++++---
> 1 file changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index 3130d010b7ad..8e07b92403ab 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2164,15 +2164,47 @@ static int neightbl_set(struct sk_buff *skb, struct nlmsghdr *nlh,
> return err;
> }
>
> +static int neightbl_valid_dump_info(const struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct ndtmsg *ndtm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ndtm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + ndtm = nlmsg_data(nlh);
> + if (ndtm->ndtm_pad1 || ndtm->ndtm_pad2) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*ndtm))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> int family, tidx, nidx = 0;
> int tbl_skip = cb->args[0];
> int neigh_skip = cb->args[1];
> struct neigh_table *tbl;
>
> - family = ((struct rtgenmsg *) nlmsg_data(cb->nlh))->rtgen_family;
> + if (cb->strict_check) {
> + int err = neightbl_valid_dump_info(nlh, cb->extack);
> +
> + if (err)
> + return err;
> + }
> +
> + family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
So this already was a problem prior to your patch: what happens when you
pass in the wrong struct? Then this case is not safe to do and might
contain all kinds of crap.
>
> for (tidx = 0; tidx < NEIGH_NR_TABLES; tidx++) {
> struct neigh_parms *p;
> @@ -2185,7 +2217,7 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
> continue;
>
> if (neightbl_fill_info(skb, tbl, NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq, RTM_NEWNEIGHTBL,
> + nlh->nlmsg_seq, RTM_NEWNEIGHTBL,
> NLM_F_MULTI) < 0)
> break;
>
> @@ -2200,7 +2232,7 @@ static int neightbl_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
>
> if (neightbl_fill_param_info(skb, tbl, p,
> NETLINK_CB(cb->skb).portid,
> - cb->nlh->nlmsg_seq,
> + nlh->nlmsg_seq,
> RTM_NEWNEIGHTBL,
> NLM_F_MULTI) < 0)
> goto out;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 14/20] net/neighbor: Update neigh_dump_info for strict data checking
From: Christian Brauner @ 2018-10-07 10:46 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-15-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:49PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update neigh_dump_info for strict data checking. If the flag is set,
> the dump request is expected to have an ndmsg struct as the header
> potentially followed by one or more attributes. Any data passed in the
> header or as an attribute is taken as a request to influence the data
> returned. Only values supported by the dump handler are allowed to be
> non-0 or set in the request. At the moment only the NDA_IFINDEX and
> NDA_MASTER attributes are supported.
>
> Existing code does not fail the dump if nlmsg_parse fails. That behavior
> is kept for non-strict checking.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/core/neighbour.c | 59 ++++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 48 insertions(+), 11 deletions(-)
>
> diff --git a/net/core/neighbour.c b/net/core/neighbour.c
> index b06f794bf91e..3130d010b7ad 100644
> --- a/net/core/neighbour.c
> +++ b/net/core/neighbour.c
> @@ -2428,13 +2428,14 @@ static int pneigh_dump_table(struct neigh_table *tbl, struct sk_buff *skb,
>
> static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> const struct nlmsghdr *nlh = cb->nlh;
> struct neigh_dump_filter filter = {};
> struct nlattr *tb[NDA_MAX + 1];
> struct neigh_table *tbl;
> int t, family, s_t;
> int proxy = 0;
> - int err;
> + int err, i;
>
> family = ((struct rtgenmsg *)nlmsg_data(nlh))->rtgen_family;
>
> @@ -2445,20 +2446,56 @@ static int neigh_dump_info(struct sk_buff *skb, struct netlink_callback *cb)
> ((struct ndmsg *)nlmsg_data(nlh))->ndm_flags == NTF_PROXY)
> proxy = 1;
>
> - err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb, NDA_MAX, NULL,
> - cb->extack);
So right, this seems like there was precedent in the kernel where an
nlmsg_parse() would fail and a warning as generated in the logs.
Anyway, as we have decided to not cause such warnings to be printed it
would make sense to make the whole nlmsg_parse() conditional and move it
under the if (strict) branch.
> - if (!err) {
> - if (tb[NDA_IFINDEX]) {
> - if (nla_len(tb[NDA_IFINDEX]) != sizeof(u32))
> - return -EINVAL;
> - filter.dev_idx = nla_get_u32(tb[NDA_IFINDEX]);
> + if (cb->strict_check) {
> + struct ndmsg *ndm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ndm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + ndm = nlmsg_data(nlh);
> + if (ndm->ndm_pad1 || ndm->ndm_pad2 || ndm->ndm_ifindex ||
> + ndm->ndm_state || ndm->ndm_flags || ndm->ndm_type) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> }
> - if (tb[NDA_MASTER]) {
> - if (nla_len(tb[NDA_MASTER]) != sizeof(u32))
> + }
> +
> + err = nlmsg_parse(nlh, sizeof(struct ndmsg), tb, NDA_MAX, NULL, extack);
> + if (err < 0) {
> + if (cb->strict_check)
> + return -EINVAL;
> + goto walk_entries;
> + }
> +
> + for (i = 0; i <= NDA_MAX; ++i) {
> + if (!tb[i])
> + continue;
> + switch (i) {
> + case NDA_IFINDEX:
> + if (nla_len(tb[i]) != sizeof(u32)) {
> + NL_SET_ERR_MSG(extack, "Invalid IFINDEX attribute");
> return -EINVAL;
> - filter.master_idx = nla_get_u32(tb[NDA_MASTER]);
> + }
> + filter.dev_idx = nla_get_u32(tb[i]);
> + break;
> + case NDA_MASTER:
> + if (nla_len(tb[i]) != sizeof(u32)) {
> + NL_SET_ERR_MSG(extack, "Invalid MASTER attribute");
> + return -EINVAL;
> + }
> + filter.master_idx = nla_get_u32(tb[i]);
> + break;
> + default:
> + if (cb->strict_check) {
> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> + return -EINVAL;
> + }
> }
> }
> +
> +walk_entries:
> s_t = cb->args[0];
>
> for (t = 0; t < NEIGH_NR_TABLES; t++) {
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 13/20] rtnetlink: Update fib dumps for strict data checking
From: Christian Brauner @ 2018-10-07 10:43 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-14-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:48PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Add helper to check netlink message for route dumps. If the strict flag
> is set the dump request is expected to have an rtmsg struct as the header.
> All elements of the struct are expected to be 0 with the exception of
> rtm_flags (which is used by both ipv4 and ipv6 dumps) and no attributes
> can be appended. rtm_flags can only have RTM_F_CLONED and RTM_F_PREFIX
> set.
>
> Update inet_dump_fib, inet6_dump_fib, mpls_dump_routes, ipmr_rtm_dumproute,
> and ip6mr_rtm_dumproute to call this helper if strict data checking is
> enabled.
This also looks good to me apart from the \n after every:
+ type <err> = foo();
+
+ if (<err> < 0)
/* handle error */
I haven't seen this style in the code a lot or I haven't looked close
enough. It just seems visually cleaner to have this close together
without a newline:
+ type <err> = foo();
+ if (<err> < 0)
/* handle error */
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> include/net/ip_fib.h | 2 ++
> net/ipv4/fib_frontend.c | 43 +++++++++++++++++++++++++++++++++++++++++--
> net/ipv4/ipmr.c | 9 +++++++++
> net/ipv6/ip6_fib.c | 8 ++++++++
> net/ipv6/ip6mr.c | 9 +++++++++
> net/mpls/af_mpls.c | 8 ++++++++
> 6 files changed, 77 insertions(+), 2 deletions(-)
>
> diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
> index f7c109e37298..9846b79c9ee1 100644
> --- a/include/net/ip_fib.h
> +++ b/include/net/ip_fib.h
> @@ -452,4 +452,6 @@ static inline void fib_proc_exit(struct net *net)
>
> u32 ip_mtu_from_fib_result(struct fib_result *res, __be32 daddr);
>
> +int ip_valid_fib_dump_req(const struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack);
> #endif /* _NET_FIB_H */
> diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
> index 30e2bcc3ef2a..1583ec0a5154 100644
> --- a/net/ipv4/fib_frontend.c
> +++ b/net/ipv4/fib_frontend.c
> @@ -802,8 +802,41 @@ static int inet_rtm_newroute(struct sk_buff *skb, struct nlmsghdr *nlh,
> return err;
> }
>
> +int ip_valid_fib_dump_req(const struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct rtmsg *rtm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*rtm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + rtm = nlmsg_data(nlh);
> + if (rtm->rtm_dst_len || rtm->rtm_src_len || rtm->rtm_tos ||
> + rtm->rtm_table || rtm->rtm_protocol || rtm->rtm_scope ||
> + rtm->rtm_type) {
> + NL_SET_ERR_MSG(extack,
> + "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> +
> + if (rtm->rtm_flags & ~(RTM_F_CLONED | RTM_F_PREFIX)) {
> + NL_SET_ERR_MSG(extack, "Invalid flags for dump request");
> + return -EINVAL;
> + }
> + if (nlh->nlmsg_len != nlmsg_msg_size(sizeof(*rtm))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ip_valid_fib_dump_req);
> +
> static int inet_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> unsigned int h, s_h;
> unsigned int e = 0, s_e;
> @@ -811,8 +844,14 @@ static int inet_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
> struct hlist_head *head;
> int dumped = 0, err;
>
> - if (nlmsg_len(cb->nlh) >= sizeof(struct rtmsg) &&
> - ((struct rtmsg *) nlmsg_data(cb->nlh))->rtm_flags & RTM_F_CLONED)
> + if (cb->strict_check) {
> + err = ip_valid_fib_dump_req(nlh, cb->extack);
> + if (err)
> + return err;
> + }
> +
> + if (nlmsg_len(nlh) >= sizeof(struct rtmsg) &&
> + ((struct rtmsg *)nlmsg_data(nlh))->rtm_flags & RTM_F_CLONED)
> return skb->len;
>
> s_h = cb->args[0];
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index e6c48e08d53d..2a7963beecfb 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -2527,6 +2527,15 @@ static int ipmr_rtm_getroute(struct sk_buff *in_skb, struct nlmsghdr *nlh,
>
> static int ipmr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> +
> + if (cb->strict_check) {
> + int err = ip_valid_fib_dump_req(nlh, cb->extack);
> +
> + if (err)
> + return err;
> + }
> +
> return mr_rtm_dumproute(skb, cb, ipmr_mr_table_iter,
> _ipmr_fill_mroute, &mfc_unres_lock);
> }
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 5516f55e214b..123786684476 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -568,6 +568,7 @@ static int fib6_dump_table(struct fib6_table *table, struct sk_buff *skb,
>
> static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> unsigned int h, s_h;
> unsigned int e = 0, s_e;
> @@ -577,6 +578,13 @@ static int inet6_dump_fib(struct sk_buff *skb, struct netlink_callback *cb)
> struct hlist_head *head;
> int res = 0;
>
> + if (cb->strict_check) {
> + int err = ip_valid_fib_dump_req(nlh, cb->extack);
> +
> + if (err)
> + return err;
> + }
> +
> s_h = cb->args[0];
> s_e = cb->args[1];
>
> diff --git a/net/ipv6/ip6mr.c b/net/ipv6/ip6mr.c
> index 6f07b8380425..8a94500c5532 100644
> --- a/net/ipv6/ip6mr.c
> +++ b/net/ipv6/ip6mr.c
> @@ -2457,6 +2457,15 @@ static void mrt6msg_netlink_event(struct mr_table *mrt, struct sk_buff *pkt)
>
> static int ip6mr_rtm_dumproute(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> +
> + if (cb->strict_check) {
> + int err = ip_valid_fib_dump_req(nlh, cb->extack);
> +
> + if (err)
> + return err;
> + }
> +
> return mr_rtm_dumproute(skb, cb, ip6mr_mr_table_iter,
> _ip6mr_fill_mroute, &mfc_unres_lock);
> }
> diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
> index 55a30ee3d820..3e33934751b4 100644
> --- a/net/mpls/af_mpls.c
> +++ b/net/mpls/af_mpls.c
> @@ -2017,6 +2017,7 @@ static int mpls_dump_route(struct sk_buff *skb, u32 portid, u32 seq, int event,
>
> static int mpls_dump_routes(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> struct mpls_route __rcu **platform_label;
> size_t platform_labels;
> @@ -2024,6 +2025,13 @@ static int mpls_dump_routes(struct sk_buff *skb, struct netlink_callback *cb)
>
> ASSERT_RTNL();
>
> + if (cb->strict_check) {
> + int err = ip_valid_fib_dump_req(nlh, cb->extack);
> +
> + if (err)
> + return err;
> + }
> +
> index = cb->args[0];
> if (index < MPLS_LABEL_FIRST_UNRESERVED)
> index = MPLS_LABEL_FIRST_UNRESERVED;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 12/20] rtnetlink: Update ipmr_rtm_dumplink for strict data checking
From: Christian Brauner @ 2018-10-07 10:40 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-13-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:47PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update ipmr_rtm_dumplink for strict data checking. If the flag is set,
> the dump request is expected to have an ifinfomsg struct as the header.
> All elements of the struct are expected to be 0 and no attributes can
> be appended.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
Just one really tiny nit below. :)
Acked-by: Christian Brauner <christian@brauner.io>
> ---
> net/ipv4/ipmr.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/net/ipv4/ipmr.c b/net/ipv4/ipmr.c
> index 5660adcf7a04..e6c48e08d53d 100644
> --- a/net/ipv4/ipmr.c
> +++ b/net/ipv4/ipmr.c
> @@ -2710,6 +2710,31 @@ static bool ipmr_fill_vif(struct mr_table *mrt, u32 vifid, struct sk_buff *skb)
> return true;
> }
>
> +static int ipmr_valid_dumplink(const struct nlmsghdr *nlh,
> + struct netlink_ext_ack *extack)
> +{
> + struct ifinfomsg *ifm;
> +
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + if (nlh->nlmsg_len > nlmsg_msg_size(sizeof(*ifm))) {
> + NL_SET_ERR_MSG(extack, "Invalid data after header");
> + return -EINVAL;
> + }
> +
> + ifm = nlmsg_data(nlh);
> + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags ||
> + ifm->ifi_change || ifm->ifi_index) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb)
> {
> struct net *net = sock_net(skb->sk);
> @@ -2718,6 +2743,13 @@ static int ipmr_rtm_dumplink(struct sk_buff *skb, struct netlink_callback *cb)
> unsigned int e = 0, s_e;
> struct mr_table *mrt;
>
> + if (cb->strict_check) {
> + int err = ipmr_valid_dumplink(cb->nlh, cb->extack);
> +
> + if (err)
> + return err;
Nit: can we remove the unnecessary \n, please.
> + }
> +
> s_t = cb->args[0];
> s_e = cb->args[1];
>
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 10/20] rtnetlink: Update rtnl_stats_dump for strict data checking
From: Christian Brauner @ 2018-10-07 10:38 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-11-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:45PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update rtnl_stats_dump for strict data checking. If the flag is set,
> the dump request is expected to have an if_stats_msg struct as the header.
> All elements of the struct are expected to be 0 except filter_mask which
> must be non-0 (legacy behavior). No attributes are supported.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/core/rtnetlink.c | 24 ++++++++++++++++++++++--
> 1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index d2c8d41a6fbc..6cdd9251771a 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4643,6 +4643,7 @@ static int rtnl_stats_get(struct sk_buff *skb, struct nlmsghdr *nlh,
>
> static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> int h, s_h, err, s_idx, s_idxattr, s_prividx;
> struct net *net = sock_net(skb->sk);
> unsigned int flags = NLM_F_MULTI;
> @@ -4659,13 +4660,32 @@ static int rtnl_stats_dump(struct sk_buff *skb, struct netlink_callback *cb)
>
> cb->seq = net->dev_base_seq;
>
> - if (nlmsg_len(cb->nlh) < sizeof(*ifsm))
> + if (nlmsg_len(cb->nlh) < sizeof(*ifsm)) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> return -EINVAL;
> + }
>
> ifsm = nlmsg_data(cb->nlh);
> +
> + /* only requests using NLM_F_DUMP_PROPER_HDR can pass data to
> + * influence the dump. The legacy exception is filter_mask.
> + */
> + if (cb->strict_check) {
> + if (ifsm->pad1 || ifsm->pad2 || ifsm->ifindex) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> + if (cb->nlh->nlmsg_len > nlmsg_msg_size(sizeof(*ifsm))) {
Nit: \n appreciated :)
> + NL_SET_ERR_MSG(extack, "Invalid attributes after header");
> + return -EINVAL;
> + }
> + }
> +
> filter_mask = ifsm->filter_mask;
> - if (!filter_mask)
> + if (!filter_mask) {
> + NL_SET_ERR_MSG(extack, "Invalid filter_mask");
Nit: probably better to have this read "Invalid filter mask".
> return -EINVAL;
> + }
>
> for (h = s_h; h < NETDEV_HASHENTRIES; h++, s_idx = 0) {
> idx = 0;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 09/20] rtnetlink: Update rtnl_bridge_getlink for strict data checking
From: Christian Brauner @ 2018-10-07 10:36 UTC (permalink / raw)
To: David Ahern; +Cc: netdev, davem, jbenc, stephen, David Ahern
In-Reply-To: <20181004213355.14899-10-dsahern@kernel.org>
On Thu, Oct 04, 2018 at 02:33:44PM -0700, David Ahern wrote:
> From: David Ahern <dsahern@gmail.com>
>
> Update rtnl_bridge_getlink for strict data checking. If the flag is set,
> the dump request is expected to have an ifinfomsg struct as the header
> potentially followed by one or more attributes. Any data passed in the
> header or as an attribute is taken as a request to influence the data
> returned. Only values supported by the dump handler are allowed to be
> non-0 or set in the request. At the moment only the IFLA_EXT_MASK
> attribute is supported.
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> ---
> net/core/rtnetlink.c | 50 ++++++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 40 insertions(+), 10 deletions(-)
>
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 4fd27b5db787..d2c8d41a6fbc 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -4000,27 +4000,57 @@ EXPORT_SYMBOL_GPL(ndo_dflt_bridge_getlink);
>
> static int rtnl_bridge_getlink(struct sk_buff *skb, struct netlink_callback *cb)
> {
> + struct netlink_ext_ack *extack = cb->extack;
> + const struct nlmsghdr *nlh = cb->nlh;
> struct net *net = sock_net(skb->sk);
> + struct nlattr *tb[IFLA_MAX+1];
> struct net_device *dev;
> int idx = 0;
> u32 portid = NETLINK_CB(cb->skb).portid;
> - u32 seq = cb->nlh->nlmsg_seq;
> + u32 seq = nlh->nlmsg_seq;
> u32 filter_mask = 0;
> - int err;
> + int err, i;
>
> - if (nlmsg_len(cb->nlh) > sizeof(struct ifinfomsg)) {
> - struct nlattr *extfilt;
> + if (cb->strict_check) {
> + struct ifinfomsg *ifm;
>
> - extfilt = nlmsg_find_attr(cb->nlh, sizeof(struct ifinfomsg),
> - IFLA_EXT_MASK);
> - if (extfilt) {
> - if (nla_len(extfilt) < sizeof(filter_mask))
> - return -EINVAL;
> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> + NL_SET_ERR_MSG(extack, "Invalid header");
> + return -EINVAL;
> + }
> +
> + ifm = nlmsg_data(nlh);
> + if (ifm->__ifi_pad || ifm->ifi_type || ifm->ifi_flags ||
> + ifm->ifi_change || ifm->ifi_index) {
> + NL_SET_ERR_MSG(extack, "Invalid values in header for dump request");
> + return -EINVAL;
> + }
> + }
>
> - filter_mask = nla_get_u32(extfilt);
> + err = nlmsg_parse(nlh, sizeof(struct ifinfomsg), tb, IFLA_MAX,
> + ifla_policy, extack);
> + if (err < 0) {
> + if (cb->strict_check)
> + return -EINVAL;
> + goto walk_entries;
> + }
What's the point of moving this out of the
if (cb->strict_check) {} branch above? This looks like it would cause
the same parse warnings that we're trying to get rid of in inet{4,6}
dumps.
Seems to make more sense to make the nlmsg_parse() itself conditional as
well unless I'm lacking context.
> +
> + for (i = 0; i <= IFLA_MAX; ++i) {
> + if (!tb[i])
> + continue;
> + switch (i) {
I'm a fan of \n between different conditions etc. so can we please have
one after the continue. :)
> + case IFLA_EXT_MASK:
> + filter_mask = nla_get_u32(tb[i]);
> + break;
> + default:
> + if (cb->strict_check) {
> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> + return -EINVAL;
> + }
> }
> }
>
> +walk_entries:
> rcu_read_lock();
> for_each_netdev_rcu(net, dev) {
> const struct net_device_ops *ops = dev->netdev_ops;
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH v2 net-next] inet: do not set backlog if listen fails
From: Eric Dumazet @ 2018-10-07 17:39 UTC (permalink / raw)
To: Yafang Shao, edumazet, davem; +Cc: netdev, linux-kernel
In-Reply-To: <1538919405-3093-1-git-send-email-laoar.shao@gmail.com>
On 10/07/2018 06:36 AM, Yafang Shao wrote:
> We don't need to set the backlog if listen fails.
> The sk_max_ack_backlog will be set in the caller inet_listen() and
> dccp_listen_start() if inet_csk_listen_start() return without error.
> So just remove this line to avoid this unnecessary operation.
>
> Regarding sk_ack_backlog, we have to set it before a TCP/DCCP socket is
> ready to accept new flows to avoid race, because dccp and tcp have lockless
> listeners
>
Really could you not add irrelevant stuff in the changelog ?
This patch simply removes one redundant setting, you do not have to explain
about dccp/tcp being lockless and that sk_ack_backlog is not touched by this patch.
Also the title is misleading, since we will still set the backlog even if the listen
fails.
If you really want sk_max_ack_backlog being not changed if listen() fails,
then I am afraid you will need to submit a different patch.
^ permalink raw reply
* Re: [PATCH net-next 08/20] rtnetlink: Update rtnl_dump_ifinfo for strict data checking
From: Christian Brauner @ 2018-10-07 10:29 UTC (permalink / raw)
To: David Ahern; +Cc: David Ahern, netdev, davem, jbenc, stephen
In-Reply-To: <cc1e134e-7d02-2f46-2a6d-5c1e823f3e97@gmail.com>
On Fri, Oct 05, 2018 at 01:22:24PM -0600, David Ahern wrote:
> On 10/5/18 11:59 AM, Christian Brauner wrote:
> >> + err = nlmsg_parse(nlh, hdrlen, tb, IFLA_MAX, ifla_policy, extack);
> >> + if (err < 0) {
> >> + if (cb->strict_check)
> >> + return -EINVAL;
> >> + goto walk_entries;
> >> + }
> >>
> >> - if (master_idx || kind_ops)
> >> - flags |= NLM_F_DUMP_FILTERED;
> >> + for (i = 0; i <= IFLA_MAX; ++i) {
> >> + if (!tb[i])
> >> + continue;
> >> + switch (i) {
> >> + case IFLA_TARGET_NETNSID:
> >> + netnsid = nla_get_s32(tb[i]);
> >> + tgt_net = rtnl_get_net_ns_capable(skb->sk, netnsid);
> >> + if (IS_ERR(tgt_net)) {
> >> + NL_SET_ERR_MSG(extack, "Invalid target namespace id");
> >> + return PTR_ERR(tgt_net);
> >> + }
> >> + break;
> >> + case IFLA_EXT_MASK:
> >> + ext_filter_mask = nla_get_u32(tb[i]);
> >> + break;
> >> + case IFLA_MASTER:
> >> + master_idx = nla_get_u32(tb[i]);
> >> + break;
> >> + case IFLA_LINKINFO:
> >> + kind_ops = linkinfo_to_kind_ops(tb[i]);
> >> + break;
> >> + default:
> >> + if (cb->strict_check) {
> >> + NL_SET_ERR_MSG(extack, "Unsupported attribute in dump request");
> >> + return -EINVAL;
> >> + }
> >> + }
> >
> > This might make sense to be split into two helpers for parsing:
> > <blablabla>_strict() and <blablabla>_lenient(). :)
>
> I thought about that, but there is so much overlap - they are mostly
> common. Besides, ifinfomsg is the header for link dumps, and ifinfomsg
> is the one that has been (ab)used for other message types, so strict
> versus lenient does not really have a differentiator for this message
> type - other than checking the elements of the struct.
It's mostly about the function being extremely long and convoluted.
Having parts moved out into (a) descriptive helper(s) with whatever name
might make this way more readable than it is now especially with the new
handling we need for strict checking.
^ permalink raw reply
* Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
From: Christian Brauner @ 2018-10-07 10:25 UTC (permalink / raw)
To: David Ahern; +Cc: David Ahern, netdev, davem, jbenc, stephen
In-Reply-To: <30b45b7e-cbda-e348-c5f1-5b78cfb84d2f@gmail.com>
On Fri, Oct 05, 2018 at 01:25:22PM -0600, David Ahern wrote:
> On 10/5/18 11:48 AM, Christian Brauner wrote:
> > On Thu, Oct 04, 2018 at 02:33:46PM -0700, David Ahern wrote:
> >> From: David Ahern <dsahern@gmail.com>
> >>
> >> Update inet6_dump_ifinfo for strict data checking. If the flag is
> >> set, the dump request is expected to have an ifinfomsg struct as
> >> the header. All elements of the struct are expected to be 0 and no
> >> attributes can be appended.
> >>
> >> Signed-off-by: David Ahern <dsahern@gmail.com>
Acked-by: Christian Brauner <christian@brauner.io>
> > This is on top of current net-next? Are your patches ensuring that
> > ipv6 addr requests don't generate log messages anymore when a wrong
> > header is passed but the strict socket option is not passed? The context
> > here doesn't seem to indicate that. :)
> >
>
> this is an AF_INET6 GETLINK handler. Why? no idea, but I think you are
> confusing this patch with the GETADDR patch which generated the
> "netlink: 16 bytes leftover after parsing attributes in process `ip'."
> message before this set.
Yes, I realized this immediately afterwards.
^ permalink raw reply
* Re: pull-request: wireless-drivers-next 2018-10-07
From: David Miller @ 2018-10-07 17:32 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <87murqdx7h.fsf@kamboji.qca.qualcomm.com>
From: Kalle Valo <kvalo@codeaurora.org>
Date: Sun, 07 Oct 2018 10:37:06 +0300
> another pull request to net-next for 4.20. I'm sending this a bit
> earlier than I prefer as I'm not sure if the merge window starts today
> or not. Not all of these patches have been in linux-next, and also the
> kbuild bot has been offline this week due to a service break, so there
> might be some build problems which I have missed. Though a local
> allmodconfig build with GCC 7.3.0 did work without problems.
Pulled and crossing fingers :-)
Thanks.
^ permalink raw reply
* Re: [PATCH net-next 11/20] rtnetlink: Update inet6_dump_ifinfo for strict data checking
From: Christian Brauner @ 2018-10-07 10:23 UTC (permalink / raw)
To: David Ahern; +Cc: David Ahern, netdev, davem, jbenc, stephen
In-Reply-To: <905b43c9-5bf0-1a64-4fda-bfee9dfe99d8@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 840 bytes --]
On Fri, Oct 05, 2018 at 01:26:31PM -0600, David Ahern wrote:
> On 10/5/18 11:54 AM, Christian Brauner wrote:
> >> +static int inet6_valid_dump_ifinfo(const struct nlmsghdr *nlh,
> >> + struct netlink_ext_ack *extack)
> >> +{
> >> + struct ifinfomsg *ifm;
> >> +
> >> + if (nlh->nlmsg_len < nlmsg_msg_size(sizeof(*ifm))) {
> >> + NL_SET_ERR_MSG(extack, "Invalid header");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + if (nlh->nlmsg_len > nlmsg_msg_size(sizeof(*ifm))) {
> >
> > Shouldn't ipv6 specific dump requests at least support IFA_TARGET_NETNSID?
>
> It does not today. The AF_UNSPEC GETLINK dumps it but the AF_INET6 does
> not.
>
> Some one can add it later if desired.
Weird, I thought I had sent a patch for that as well. Doesn't matter now
I'll just send one once your branch lands. :) Thanks!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Jason A. Donenfeld @ 2018-10-07 17:26 UTC (permalink / raw)
To: Jiří Pírko; +Cc: LKML, Netdev, David Miller, Greg Kroah-Hartman
In-Reply-To: <20181006065819.GD3061@nanopsycho.orion>
Hi Jiri,
On Sat, Oct 6, 2018 at 9:03 AM Jiri Pirko <jiri@resnulli.us> wrote:
> >+
> >+ wg->incoming_handshakes_worker =
> >+ wg_packet_alloc_percpu_multicore_worker(
> >+ wg_packet_handshake_receive_worker, wg);
> >+ if (!wg->incoming_handshakes_worker)
> >+ goto error_2;
>
>
> Please consider renaming the label to "what went wrong". In this case,
> it would be "err_alloc_worker".
>
>
> >+
> >+ wg->handshake_receive_wq = alloc_workqueue("wg-kex-%s",
> >+ WQ_CPU_INTENSIVE | WQ_FREEZABLE, 0, dev->name);
> >+ if (!wg->handshake_receive_wq)
> >+ goto error_3;
> >+
> >+ wg->handshake_send_wq = alloc_workqueue("wg-kex-%s",
> >+ WQ_UNBOUND | WQ_FREEZABLE, 0, dev->name);
> >+ if (!wg->handshake_send_wq)
> >+ goto error_4;
> >+
> >+ wg->packet_crypt_wq = alloc_workqueue("wg-crypt-%s",
> >+ WQ_CPU_INTENSIVE | WQ_MEM_RECLAIM, 0, dev->name);
> >+ if (!wg->packet_crypt_wq)
> >+ goto error_5;
> >+
> >+ if (wg_packet_queue_init(&wg->encrypt_queue, wg_packet_encrypt_worker,
> >+ true, MAX_QUEUED_PACKETS) < 0)
>
> You need to have "int err" and always in cases like this to do:
> err = wg_packet_queue_init()
> if (err)
> goto err_*
>
>
> >+ goto error_6;
> >+
> >+ if (wg_packet_queue_init(&wg->decrypt_queue, wg_packet_decrypt_worker,
> >+ true, MAX_QUEUED_PACKETS) < 0)
> >+ goto error_7;
> >+
> >+ ret = wg_ratelimiter_init();
> >+ if (ret < 0)
> >+ goto error_8;
> >+
> >+ ret = register_netdevice(dev);
> >+ if (ret < 0)
> >+ goto error_9;
> >+
> >+ list_add(&wg->device_list, &device_list);
> >+
> >+ /* We wait until the end to assign priv_destructor, so that
> >+ * register_netdevice doesn't call it for us if it fails.
> >+ */
> >+ dev->priv_destructor = destruct;
> >+
> >+ pr_debug("%s: Interface created\n", dev->name);
> >+ return ret;
> >+
> >+error_9:
> >+ wg_ratelimiter_uninit();
> >+error_8:
> >+ wg_packet_queue_free(&wg->decrypt_queue, true);
> >+error_7:
> >+ wg_packet_queue_free(&wg->encrypt_queue, true);
> >+error_6:
> >+ destroy_workqueue(wg->packet_crypt_wq);
> >+error_5:
> >+ destroy_workqueue(wg->handshake_send_wq);
> >+error_4:
> >+ destroy_workqueue(wg->handshake_receive_wq);
> >+error_3:
> >+ free_percpu(wg->incoming_handshakes_worker);
> >+error_2:
> >+ free_percpu(dev->tstats);
> >+error_1:
> >+ return ret;
> >+}
I'll change away from using error_9,8,7,6,5,4,3,2,1 because of your
suggestion -- and because it's the norm in the kernel to use real
names. But, I would be interested in your opinion on the numerical
errors' reasoning for existing in the first place. The idea was that
with so many different failure cases that need to cascade in the
correct order, it's much easier to visually inspect that it's been
done right by observing up top 1,2,3,4,5,6,7,8,9 and at the bottom
9,8,7,6,5,4,3,2,1, rather than having to store in my brain's limited
stack space what each name pertains to and keep track of the ordering
and such. In light of that, do you still think that following the
convention of textual error labels is a good match here? Again, I'm
changing this for v8, but I am nonetheless curious about what you
think.
Jason
^ permalink raw reply
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Jason A. Donenfeld @ 2018-10-07 16:55 UTC (permalink / raw)
To: Andrew Lunn; +Cc: LKML, Netdev, David Miller, Greg Kroah-Hartman
In-Reply-To: <20181007164807.GA21302@lunn.ch>
Hi Andrew,
On Sun, Oct 7, 2018 at 6:48 PM Andrew Lunn <andrew@lunn.ch> wrote:
> Hi Jason
>
> This is the sort of thing you should state in the patchset version
> history. It is O.K. to say i will address this later, but you need to
> communicate that. Otherwise reviewers just get frustrated that
> comments are getting repeatedly ignored and why should they bother
> reviewing it. And without reviewers willing to review your code, it
> will never get into mainline.
Understood. Sorry about that.
Jason
^ permalink raw reply
* Re: [PATCH net-next v7 28/28] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-10-07 16:48 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: LKML, Netdev, David Miller, Greg Kroah-Hartman
In-Reply-To: <CAHmME9ocMekP+jauihMGKq04to0_jQTjhv=r2wVEcqmLtKJu2A@mail.gmail.com>
On Sun, Oct 07, 2018 at 03:57:38AM +0200, Jason A. Donenfeld wrote:
> Hey Andrew,
>
> On Sun, Oct 7, 2018 at 2:01 AM Andrew Lunn <andrew@lunn.ch> wrote:
> > The BUG() statements all seemed to be removed. Thanks.
> >
> > We still have 1 errors, 193 warnings, 7529 lines checked from
> > checkpatch. There are still some Macros flow control statements in
> > them, despite me pointed them out multiple times.
> >
> > Im not reviewing this version any further. Until you get the very
> > basics right, i doubt this patch is going to make much progress
> > towards inclusion.
>
> Dave mentioned he wanted the Zinc stuff to be all set before looking
> at this, so I've been focusing my efforts mostly on that. But I
> haven't forgotten about your reviews, and seeing as you're actually
> interested in looking at this now, I'll be sure to address everything
> you've mentioned for v8. When I'm reasonably sure that's all done,
> I'll let you know, so that you're not disappointed again.
Hi Jason
This is the sort of thing you should state in the patchset version
history. It is O.K. to say i will address this later, but you need to
communicate that. Otherwise reviewers just get frustrated that
comments are getting repeatedly ignored and why should they bother
reviewing it. And without reviewers willing to review your code, it
will never get into mainline.
^ permalink raw reply
* [RFC PATCH net 3/3] uapi, net/smc: provide socket state constants in UAPI
From: Eugene Syromiatnikov @ 2018-10-07 16:35 UTC (permalink / raw)
To: netdev
Cc: David S. Miller, linux-kernel, Ursula Braun, Karsten Graul,
Hans Wippel, linux-s390
In-Reply-To: <cover.1538929504.git.esyr@redhat.com>
As socket state itself is already exposed via sock_diag interface.
Fixes: ac7138746e14 ("smc: establish new socket family")
Fixes: b38d732477e4 ("smc: socket closing and linkgroup cleanup")
Signed-off-by: Eugene Syromiatnikov <esyr@redhat.com>
---
include/uapi/linux/smc_diag.h | 17 +++++++++++++++++
net/smc/smc.h | 18 +-----------------
2 files changed, 18 insertions(+), 17 deletions(-)
diff --git a/include/uapi/linux/smc_diag.h b/include/uapi/linux/smc_diag.h
index 8cb3a6f..6798ec0 100644
--- a/include/uapi/linux/smc_diag.h
+++ b/include/uapi/linux/smc_diag.h
@@ -31,6 +31,23 @@ struct smc_diag_msg {
__aligned_u64 diag_inode;
};
+enum smc_state { /* possible states of an SMC socket */
+ SMC_ACTIVE = 1,
+ SMC_INIT = 2,
+ SMC_CLOSED = 7,
+ SMC_LISTEN = 10,
+ /* normal close */
+ SMC_PEERCLOSEWAIT1 = 20,
+ SMC_PEERCLOSEWAIT2 = 21,
+ SMC_APPFINCLOSEWAIT = 24,
+ SMC_APPCLOSEWAIT1 = 22,
+ SMC_APPCLOSEWAIT2 = 23,
+ SMC_PEERFINCLOSEWAIT = 25,
+ /* abnormal close */
+ SMC_PEERABORTWAIT = 26,
+ SMC_PROCESSABORT = 27,
+};
+
/* Mode of a connection */
enum {
SMC_DIAG_MODE_SMCR,
diff --git a/net/smc/smc.h b/net/smc/smc.h
index f5ff2ee..16bfd64 100644
--- a/net/smc/smc.h
+++ b/net/smc/smc.h
@@ -16,6 +16,7 @@
#include <linux/compiler.h> /* __aligned */
#include <net/sock.h>
#include <linux/smc.h>
+#include <linux/smc_diag.h>
#include "smc_ib.h"
@@ -26,23 +27,6 @@ extern struct proto smc_proto6;
#define KERNEL_HAS_ATOMIC64
#endif
-enum smc_state { /* possible states of an SMC socket */
- SMC_ACTIVE = 1,
- SMC_INIT = 2,
- SMC_CLOSED = 7,
- SMC_LISTEN = 10,
- /* normal close */
- SMC_PEERCLOSEWAIT1 = 20,
- SMC_PEERCLOSEWAIT2 = 21,
- SMC_APPFINCLOSEWAIT = 24,
- SMC_APPCLOSEWAIT1 = 22,
- SMC_APPCLOSEWAIT2 = 23,
- SMC_PEERFINCLOSEWAIT = 25,
- /* abnormal close */
- SMC_PEERABORTWAIT = 26,
- SMC_PROCESSABORT = 27,
-};
-
struct smc_link_group;
struct smc_wr_rx_hdr { /* common prefix part of LLC and CDC to demultiplex */
--
2.1.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