* Re: [PATCH] netfilter: nf_tables: add SECMARK support
From: Casey Schaufler @ 2018-09-19 23:36 UTC (permalink / raw)
To: Christian Göttsche, pablo, kadlec, fw, davem,
netfilter-devel, coreteam, netdev, linux-kernel, paul, sds,
eparis, jmorris, serge, selinux, linux-security-module
In-Reply-To: <20180919231402.4482-1-cgzones@googlemail.com>
On 9/19/2018 4:14 PM, Christian Göttsche wrote:
> Add the ability to set the security context of packets within the nf_tables framework.
> Add a nft_object for holding security contexts in the kernel and manipulating packets on the wire.
> The contexts are kept as strings and are evaluated to security identifiers at runtime (packet arrival),
> so that the nft_objects do not need to be refreshed after security changes.
> The maximum security context length is set to 256.
>
> Based on v4.18.6
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
I've only had a cursory look at your patch, but how is it
different from what's in xt_SECMARK.c ?
> ---
> include/net/netfilter/nf_tables_core.h | 4 +
> include/uapi/linux/netfilter/nf_tables.h | 18 ++++-
> net/netfilter/nf_tables_core.c | 28 ++++++-
> net/netfilter/nft_meta.c | 95 ++++++++++++++++++++++++
> 4 files changed, 140 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/netfilter/nf_tables_core.h b/include/net/netfilter/nf_tables_core.h
> index a0513450..0d1f3b96 100644
> --- a/include/net/netfilter/nf_tables_core.h
> +++ b/include/net/netfilter/nf_tables_core.h
> @@ -16,6 +16,10 @@ extern struct nft_expr_type nft_meta_type;
> extern struct nft_expr_type nft_rt_type;
> extern struct nft_expr_type nft_exthdr_type;
>
> +#ifdef CONFIG_NETWORK_SECMARK
> +extern struct nft_object_type nft_secmark_obj_type;
> +#endif
> +
> int nf_tables_core_module_init(void);
> void nf_tables_core_module_exit(void);
>
> diff --git a/include/uapi/linux/netfilter/nf_tables.h b/include/uapi/linux/netfilter/nf_tables.h
> index 89438e68..f1527962 100644
> --- a/include/uapi/linux/netfilter/nf_tables.h
> +++ b/include/uapi/linux/netfilter/nf_tables.h
> @@ -1169,6 +1169,21 @@ enum nft_quota_attributes {
> };
> #define NFTA_QUOTA_MAX (__NFTA_QUOTA_MAX - 1)
>
> +/**
> + * enum nft_secmark_attributes - nf_tables secmark object netlink attributes
> + *
> + * @NFTA_SECMARK_CTX: security context (NLA_STRING)
> + */
> +enum nft_secmark_attributes {
> + NFTA_SECMARK_UNSPEC,
> + NFTA_SECMARK_CTX,
> + __NFTA_SECMARK_MAX,
> +};
> +#define NFTA_SECMARK_MAX (__NFTA_SECMARK_MAX - 1)
> +
> +/* Max security context length */
> +#define NFT_SECMARK_CTX_MAXLEN 256
> +
> /**
> * enum nft_reject_types - nf_tables reject expression reject types
> *
> @@ -1398,7 +1413,8 @@ enum nft_ct_helper_attributes {
> #define NFT_OBJECT_CT_HELPER 3
> #define NFT_OBJECT_LIMIT 4
> #define NFT_OBJECT_CONNLIMIT 5
> -#define __NFT_OBJECT_MAX 6
> +#define NFT_OBJECT_SECMARK 6
> +#define __NFT_OBJECT_MAX 7
> #define NFT_OBJECT_MAX (__NFT_OBJECT_MAX - 1)
>
> /**
> diff --git a/net/netfilter/nf_tables_core.c b/net/netfilter/nf_tables_core.c
> index 8de912ca..d59ebba0 100644
> --- a/net/netfilter/nf_tables_core.c
> +++ b/net/netfilter/nf_tables_core.c
> @@ -235,12 +235,24 @@ static struct nft_expr_type *nft_basic_types[] = {
> &nft_exthdr_type,
> };
>
> +static struct nft_object_type *nft_basic_objects[] = {
> +#ifdef CONFIG_NETWORK_SECMARK
> + &nft_secmark_obj_type,
> +#endif
> +};
> +
> int __init nf_tables_core_module_init(void)
> {
> - int err, i;
> + int err, i, j = 0;
> +
> + for (i = 0; i < ARRAY_SIZE(nft_basic_objects); i++) {
> + err = nft_register_obj(nft_basic_objects[i]);
> + if (err)
> + goto err;
> + }
>
> - for (i = 0; i < ARRAY_SIZE(nft_basic_types); i++) {
> - err = nft_register_expr(nft_basic_types[i]);
> + for (j = 0; j < ARRAY_SIZE(nft_basic_types); j++) {
> + err = nft_register_expr(nft_basic_types[j]);
> if (err)
> goto err;
> }
> @@ -248,8 +260,12 @@ int __init nf_tables_core_module_init(void)
> return 0;
>
> err:
> + while (j-- > 0)
> + nft_unregister_expr(nft_basic_types[j]);
> +
> while (i-- > 0)
> - nft_unregister_expr(nft_basic_types[i]);
> + nft_unregister_obj(nft_basic_objects[i]);
> +
> return err;
> }
>
> @@ -260,4 +276,8 @@ void nf_tables_core_module_exit(void)
> i = ARRAY_SIZE(nft_basic_types);
> while (i-- > 0)
> nft_unregister_expr(nft_basic_types[i]);
> +
> + i = ARRAY_SIZE(nft_basic_objects);
> + while (i-- > 0)
> + nft_unregister_obj(nft_basic_objects[i]);
> }
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 1105a23b..26b79a3c 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -540,3 +540,98 @@ struct nft_expr_type nft_meta_type __read_mostly = {
> .maxattr = NFTA_META_MAX,
> .owner = THIS_MODULE,
> };
> +
> +#ifdef CONFIG_NETWORK_SECMARK
> +
> +struct nft_secmark {
> + char ctx[NFT_SECMARK_CTX_MAXLEN];
> + int len;
> +};
> +
> +static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
> + [NFTA_SECMARK_CTX] = { .type = NLA_STRING, .len = NFT_SECMARK_CTX_MAXLEN },
> +};
> +
> +
> +static void nft_secmark_obj_eval(struct nft_object *obj, struct nft_regs *regs, const struct nft_pktinfo *pkt)
> +{
> + const struct nft_secmark *priv = nft_obj_data(obj);
> + struct sk_buff *skb = pkt->skb;
> + int err;
> + u32 secid = 0;
> +
> + /* skip if packet has already a secmark */
> + if (skb->secmark)
> + return;
> +
> + err = security_secctx_to_secid(priv->ctx, priv->len, &secid);
> + if (err) {
> + if (err == -EINVAL)
> + pr_notice_ratelimited("invalid security context \'%s\'\n", priv->ctx);
> + else
> + pr_notice_ratelimited("unable to convert security context \'%s\': %d\n", priv->ctx, -err);
> + return;
> + }
> +
> + if (!secid) {
> + pr_notice_ratelimited("unable to map security context \'%s\'\n", priv->ctx);
> + return;
> + }
> +
> + err = security_secmark_relabel_packet(secid);
> + if (err) {
> + pr_notice_ratelimited("unable to obtain relabeling permission: %d\n", -err);
> + return;
> + }
> +
> + skb->secmark = secid;
> +}
> +
> +
> +static int nft_secmark_obj_init(const struct nft_ctx *ctx, const struct nlattr * const tb[], struct nft_object *obj)
> +{
> + struct nft_secmark *priv = nft_obj_data(obj);
> +
> + if (tb[NFTA_SECMARK_CTX] == NULL)
> + return -EINVAL;
> +
> + nla_strlcpy(priv->ctx, tb[NFTA_SECMARK_CTX], NFT_SECMARK_CTX_MAXLEN);
> + priv->len = strlen(priv->ctx);
> +
> + security_secmark_refcount_inc();
> +
> + return 0;
> +}
> +
> +static int nft_secmark_obj_dump(struct sk_buff *skb, struct nft_object *obj, bool reset)
> +{
> + const struct nft_secmark *priv = nft_obj_data(obj);
> +
> + if (nla_put_string(skb, NFTA_SECMARK_CTX, priv->ctx))
> + return -1;
> +
> + return 0;
> +}
> +
> +static void nft_secmark_obj_destroy(const struct nft_ctx *ctx, struct nft_object *obj)
> +{
> + security_secmark_refcount_dec();
> +}
> +
> +static const struct nft_object_ops nft_secmark_obj_ops = {
> + .type = &nft_secmark_obj_type,
> + .size = sizeof(struct nft_secmark),
> + .init = nft_secmark_obj_init,
> + .eval = nft_secmark_obj_eval,
> + .dump = nft_secmark_obj_dump,
> + .destroy = nft_secmark_obj_destroy,
> +};
> +struct nft_object_type nft_secmark_obj_type __read_mostly = {
> + .type = NFT_OBJECT_SECMARK,
> + .ops = &nft_secmark_obj_ops,
> + .maxattr = NFTA_SECMARK_MAX,
> + .policy = nft_secmark_policy,
> + .owner = THIS_MODULE,
> +};
> +
> +#endif /* CONFIG_NETWORK_SECMARK */
^ permalink raw reply
* Re: [PATCH v8 0/4] gpiolib: speed up GPIO array processing
From: Linus Walleij @ 2018-09-19 18:08 UTC (permalink / raw)
To: Janusz Krzysztofik
Cc: Jonathan Corbet, Miguel Ojeda Sandonis, Peter Korsgaard,
Peter Rosin, Ulf Hansson, Andrew Lunn, Florian Fainelli,
David S. Miller, Dominik Brodowski, Greg KH, kishon,
Lars-Peter Clausen, Michael Hennerich, Jonathan Cameron,
Hartmut Knaack, Peter Meerwald, Jiri Slaby, Willy Tarreau,
Geert Uytterhoeven, Sebastien Bourdelin
In-Reply-To: <CACRpkdbAG1N0v-BrXruf5088L0x3XwSHbFTWOFxjHQWx0LKz5g@mail.gmail.com>
On Thu, Sep 13, 2018 at 2:22 AM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Sep 5, 2018 at 11:49 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
>
> > The goal is to boost performance of get/set array functions while
> > processing GPIO arrays which represent pins of a signle chip in
> > hardware order. If resulting performance is close to PIO, GPIO API
> > can be used for data I/O without much loss of speed.
>
> I applied the v8 to an immutable branch and pushed to kernelorg
> so the build servers can churn it a bit, and if it works fine
> then we can merge this into the devel branch and also set up
> that as something other subsystems can pull in if they need it.
>
> I'm really excited to merge this!
The branch built with no problems, and now I merged this into
devel. If that also builds fine, I will let it hit linux-next so we can
stabilize it for v4.20.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH rdma-next 00/24] Extend DEVX functionality
From: Jason Gunthorpe @ 2018-09-19 18:17 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, Leon Romanovsky, RDMA mailing list, Yishai Hadas,
Saeed Mahameed, linux-netdev
In-Reply-To: <20180917110418.18937-1-leon@kernel.org>
On Mon, Sep 17, 2018 at 02:03:53PM +0300, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> From Yishai,
>
> This series comes to enable the DEVX functionality in some wider scope,
> specifically,
> - It enables using kernel objects that were created by the verbs
> API in the DEVX flow.
> - It enables white list commands without DEVX user context.
> - It enables the IB link layer under CAP_NET_RAW capabilities.
> - It exposes the PRM handles for RAW QP (i.e. TIRN, TISN, RQN, SQN)
> to be used later on directly by the DEVX interface.
>
> In General,
> Each object that is created/destroyed/modified via verbs will be stamped
> with a UID based on its user context. This is already done for DEVX objects
> commands.
>
> This will enable the firmware to enforce the usage of kernel objects
> from the DEVX flow by validating that the same UID is used and the resources are
> really related to the same user.
>
> For example in case a CQ was created with verbs it will be stamped with
> UID and once will be pointed by a DEVX create QP command the firmware will
> validate that the input CQN really belongs to the UID which issues the create QP
> command.
>
> As of the above, all the PRM objects (except of the public ones which
> are managed by the kernel e.g. FLOW, etc.) will have a UID upon their
> create/modify/destroy commands. The detection of UMEM / physical
> addressed in the relevant commands will be done by firmware according to a 'umem
> valid bit' as the UID may be used in both cases.
>
> The series also enables white list commands which don't require a
> specific DEVX context, instead of this a device UID is used so that
> the firmware will mask un-privileged functionality. The IB link layer
> is also enabled once CAP_NET_RAW permission exists.
>
> To enable using the RAW QP underlay objects (e.g. TIRN, RQN, etc.) later
> on by DEVX commands the UHW output for this case was extended to return this
> data when a DEVX context is used.
>
> Thanks
>
> Leon Romanovsky (1):
> net/mlx5: Update mlx5_ifc with DEVX UID bits
>
> Yishai Hadas (24):
> net/mlx5: Set uid as part of CQ commands
> net/mlx5: Set uid as part of QP commands
> net/mlx5: Set uid as part of RQ commands
> net/mlx5: Set uid as part of SQ commands
> net/mlx5: Set uid as part of SRQ commands
> net/mlx5: Set uid as part of DCT commands
> IB/mlx5: Set uid as part of CQ creation
> IB/mlx5: Set uid as part of QP creation
> IB/mlx5: Set uid as part of RQ commands
> IB/mlx5: Set uid as part of SQ commands
> IB/mlx5: Set uid as part of TIR commands
> IB/mlx5: Set uid as part of TIS commands
> IB/mlx5: Set uid as part of RQT commands
> IB/mlx5: Set uid as part of PD commands
> IB/mlx5: Set uid as part of TD commands
> IB/mlx5: Set uid as part of SRQ commands
> IB/mlx5: Set uid as part of DCT commands
> IB/mlx5: Set uid as part of XRCD commands
> IB/mlx5: Set uid as part of MCG commands
This is really too many patches.. They are small and not too hard to
review, but it is well beyond the guideline.
And I'm not totally happy with the extensive use of ucontext in the IB
portions, it is problematic looking into the future, and uboject is
really not supposed to be used in the drivers.
The driver needs to store the uid in the PD (copied from the ucontext
that created it) and use that in all the dependent places, not use
pd->uobject->ucontext->devx_uid or some other convoluted way to get
to it.
The ucontext variable should only be used when creating the PD, CQ and
devx objects.
This detail becomes quite important, for instance, if we get to the
'shared pd' that has been talked about at conference. In this case
when the 'receiver' of the 'shared pd' creates a child object, like a
MR, the MR must be stamped with the devx_uid of the PD (ie the
originating context's devx_uid), not the dev_uid of its local ufile!
If we do that, then the series can be split, so long as pd->devx_uid ==
0 until the entire series is applied. uid tagging is an all-or-nothing
thing, as partial tagging will break verbs. So breaking it up also
makes it more bi-section safe.
Something like these patches:
> net/mlx5: Update mlx5_ifc with DEVX UID bits
> net/mlx5: Set uid as part of CQ commands
> net/mlx5: Set uid as part of QP commands
> net/mlx5: Set uid as part of RQ commands
> net/mlx5: Set uid as part of SQ commands
> net/mlx5: Set uid as part of SRQ commands
> net/mlx5: Set uid as part of DCT commands
> IB/mlx5: Set uid as part of PD commands
> IB/mlx5: Set uid as part of QP creation
> IB/mlx5: Set uid as part of RQ commands
> IB/mlx5: Set uid as part of SQ commands
> IB/mlx5: Set uid as part of SRQ commands
> IB/mlx5: Set uid as part of DCT commands
(13 patches)
Followed by the rest of the IB uid patches
Followed by a patch to make pd->uid != 0 along with these:
> IB/mlx5: Set uid as part of CQ creation
> IB/mlx5: Set valid umem bit on DEVX
> IB/mlx5: Expose RAW QP device handles to user space
> IB/mlx5: Manage device uid for DEVX white list commands
> IB/mlx5: Enable DEVX white list commands
> IB/mlx5: Enable DEVX on IB
Jason
^ permalink raw reply
* Re: kernels > v4.12 oops/crash with ipsec-traffic: bisected to b838d5e1c5b6e57b10ec8af2268824041e3ea911: ipv4: mark DST_NOGC and remove the operation of dst_free()
From: Tobias Hommel @ 2018-09-19 18:38 UTC (permalink / raw)
To: Steffen Klassert
Cc: Wolfgang Walter, Kristian Evensen, Network Development, weiwan,
edumazet
In-Reply-To: <20180912151823.z2wk7hnex4zxly3e@arbeitstier>
> After running for about 24 hours, I now encountered another panic. This time it
> is caused by an out of memory situation. Although the trace shows action in the
> filesystem code I'm posting it here because I cannot isolate the error and
> maybe it is caused by our NULL pointer bug or by the new fix.
> I do not have a serial console attached, so I could only attach a screenshot of
> the panic to this mail.
>
> I am running v4.19-rc3 from git with the above mentioned patch applied.
> After 19 hours everything still looked fine, XfrmFwdHdrError value was at ~950.
> Overall memory usage shown by htop was at 1.2G/15.6G.
> I had htop running via ssh so I was able to see at least some status post
> mortem. Uptime: 23:50:57
> Overall memory usage was at 10.2G/15.6G and user processes were just
> using the usual amount of memory, so it looks like the kernel was eating up at
> least 9G of RAM.
>
> Maybe this information is not very helpful for debugging, but it is at least a
> warning that something might still be wrong.
>
> I'll try to gather some more information and keep you updated.
Running stable under load for more than 5 days now, I was not able to reproduce
that OOM situation. I leave it at that, the fix for the initial bug is fine for
me.
^ permalink raw reply
* Re: [PATCH mlx5-next 03/25] net/mlx5: Set uid as part of RQ commands
From: Saeed Mahameed @ 2018-09-19 18:40 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Leon Romanovsky, Doug Ledford, Leon Romanovsky, RDMA mailing list,
Yishai Hadas, Saeed Mahameed, Linux Netdev List
In-Reply-To: <20180919172855.GN11367@ziepe.ca>
On Wed, Sep 19, 2018 at 10:28 AM Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Mon, Sep 17, 2018 at 02:03:56PM +0300, Leon Romanovsky wrote:
> > From: Yishai Hadas <yishaih@mellanox.com>
> >
> > Set uid as part of RQ commands so that the firmware can manage the
> > RQ object in a secured way.
> >
> > That will enable using an RQ that was created by verbs application
> > to be used by the DEVX flow in case the uid is equal.
> >
> > Signed-off-by: Yishai Hadas <yishaih@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/net/ethernet/mellanox/mlx5/core/qp.c | 16 ++++++++++++++--
> > include/linux/mlx5/mlx5_ifc.h | 6 +++---
> > 2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/qp.c b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> > index 04f72a1cdbcc..0ca68ef54d93 100644
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/qp.c
> > @@ -540,6 +540,17 @@ int mlx5_core_xrcd_dealloc(struct mlx5_core_dev *dev, u32 xrcdn)
> > }
> > EXPORT_SYMBOL_GPL(mlx5_core_xrcd_dealloc);
> >
> > +static void destroy_rq_tracked(struct mlx5_core_dev *dev, u32 rqn, u16 uid)
> > +{
> > + u32 in[MLX5_ST_SZ_DW(destroy_rq_in)] = {0};
> > + u32 out[MLX5_ST_SZ_DW(destroy_rq_out)] = {0};
>
> = {} is the preferred version of this, right?
>
> {0} explicitly initializes the first element to zero and only works if
> the first element happens to be something integral..
>
Both are perfectly ok in our scenarios.
I remember one of the syntaxes yielded a statistic checker warning, i
don't remember which syntax and what static checker :) ..
> Jason
^ permalink raw reply
* Re: [RFC bpf-next 2/4] bpf: return EOPNOTSUPP when map lookup isn't supported
From: Mauricio Vasquez @ 2018-09-19 18:40 UTC (permalink / raw)
To: Alexei Starovoitov, Prashant Bhole
Cc: Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski,
Quentin Monnet, David S . Miller, netdev
In-Reply-To: <20180919151422.tysdcku2hxaq37xw@ast-mbp>
On 09/19/2018 10:14 AM, Alexei Starovoitov wrote:
> On Wed, Sep 19, 2018 at 04:51:41PM +0900, Prashant Bhole wrote:
>> Return ERR_PTR(-EOPNOTSUPP) from map_lookup_elem() methods of below
>> map types:
>> - BPF_MAP_TYPE_PROG_ARRAY
>> - BPF_MAP_TYPE_STACK_TRACE
>> - BPF_MAP_TYPE_XSKMAP
>> - BPF_MAP_TYPE_SOCKMAP/BPF_MAP_TYPE_SOCKHASH
>>
>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>> ---
>> kernel/bpf/arraymap.c | 2 +-
>> kernel/bpf/sockmap.c | 2 +-
>> kernel/bpf/stackmap.c | 2 +-
>> kernel/bpf/xskmap.c | 2 +-
>> 4 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
>> index dded84cbe814..24583da9ffd1 100644
>> --- a/kernel/bpf/arraymap.c
>> +++ b/kernel/bpf/arraymap.c
>> @@ -449,7 +449,7 @@ static void fd_array_map_free(struct bpf_map *map)
>>
>> static void *fd_array_map_lookup_elem(struct bpf_map *map, void *key)
>> {
>> - return NULL;
>> + return ERR_PTR(-EOPNOTSUPP);
>> }
> conceptually the set looks good to me.
> Please add a test to test_verifier.c to make sure
> that these lookup helpers cannot be called from BPF program.
> Otherwise this diff may cause crashes.
>
>
I think we can remove all these stub functions that return EOPNOTSUPP or
EINVAL and return the error in syscall.c if ops->map_[update, delete,
lookup, ...] is null.
This will require to extend (and test) the verifier to guarantee that
those helpers are not called in wrong maps, for example
map_delete_element in array like maps.
Would it make sense?
^ permalink raw reply
* Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
From: Marcelo Ricardo Leitner @ 2018-09-19 18:46 UTC (permalink / raw)
To: Johannes Berg; +Cc: netdev
In-Reply-To: <1537349117.10305.25.camel@sipsolutions.net>
On Wed, Sep 19, 2018 at 11:25:17AM +0200, Johannes Berg wrote:
> On Wed, 2018-09-19 at 00:37 -0300, Marcelo Ricardo Leitner wrote:
>
> > Did you consider indicating the message level, and only overwrite the
> > message that is already in there if the new message level is higher
> > than the current one?
>
> Hmm, no, I guess I didn't - I'm not even sure I understand what you're
> saying.
>
> This code in itself generates no "warning" messages; that was just a
> construct we discussed in the NLA_REJECT thread, e.g. if you say (like I
> just also wrote in my reply to Jiri):
>
> NL_SET_ERR_MSG(extack, "warning: deprecated command");
> err = nla_parse(..., extack);
> if (err)
> return err;
> /* do something */
> return 0;
>
> Here you could consider the message there a warning that's transported
> out even if we return 0, but if we return with a failure from
> nla_parse() (or nla_validate instead if you wish), then that failure
> message "wins".
Agree. This is the core issue here, IMHO. Once out of the context that
set the message, we have no way of knowing if we can nor should
overwrite the message that is already in there.
>
> > This way the first to set an Error message will have it, and Warning
> > messages would be overwritten by such if needed. But it also would
> > cause the first warning to be held, and not the last one, as it does
> > today. We want the first error, but the last warning otherwise.
> >
> > It would not be possible to overwrite if new_msglvl >= cur_msglvl
> > because then it would trigger the initial issue again, so some extra
> > logic would be needed to solve this.
>
> That sounds way more complex than what I'm doing now?
Yes but hopefully it would a clearer way of how it is/should be handled.
>
> Note, like I said above, this isn't *generic* in any way. This code here
> will only ever set error messages that should "win".
>
> I suppose we could - technically - make that generic, in that we could
> have both
>
> NLA_SET_WARN_MSG(extack, "...");
> NLA_SET_ERR_MSG(extack, "...");
I like this.
>
> and keep track of warning vs. error; however, just like my first version
> of the NLA_REJECT patch, that would break existing code.
Hm, I may have missed something but I think the discussion in there
was for a different context. For an extack msg to be set by
when validate_nla() call returns on nla_parse(), the previous message
had to be a "warning" because otherwise the parsing wouldn't be even
attempted. So in that case, we are safe to simply overwrite it.
While for the situation you are describing here, it will set a generic
error message in case the inner code didn't do it.
Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
replace other WARNs but not ERRs, and ERR would replace other WARNs
too but not other ERRs. All we need to do handle this is a bit in
extack saying if the message is considered a warning or not, or an
error/fatal message or not.
>
> I also don't think that we actually *need* this complexity in general.
> It should almost always be possible (and realistically, pretty easy) to
> structure your code in a way that warning messages only go out if no
> error message overwrites them. The only reason we were ever even
> discussing this was that in NLA_REJECT I missed the fact that somebody
> could've set a message before and thus would keep it rather than
> overwrite it, which was a change in behaviour.
Okay but we have split parsing of netlink messages and this could be
useful in there too:
In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
processing, and then handle it to a classifier. cls_flower, for
example, will then do another parsing. If, for whatever reason, flower
failed and did not set an extack msg, tc_new_tfilter() could set a
default error message, but at this moment we can't tell if the msg
already set was just a warning from the first parsing (or any other
code before engaging flower) (which we can overwrite) or if it a
message that flower set (which we should not overwrite).
Hopefully my description clear.. 8-)
I think this is the same situation as with the nested parsing you're
proposing.
Currently it (tc_new_tfilter) doesn't set any default error message,
so this issue is/was not noticed.
>
> Now, with this patch, all I'm doing is changing the internal behaviour
> of nla_parse/nla_validate - externally, it still overwrites any existing
> message if an error occurs, but internally it keeps the inner-most
> error.
>
> Why is this? Consider this:
>
> static const struct nla_policy inner_policy[] = {
> [INNER_FLAG] = { .type = NLA_REJECT,
> .validation_data = "must not set this flag" }
> };
>
> static const struct nla_policy outer_policy[] = {
> [OUTER_NESTING] = { .type = NLA_NESTED, .len = INNER_MAX,
> .validation_data = inner_policy,
> };
>
> Now if you invoke nla_parse/nla_validate with a message like this
>
> [ OUTER_NESTING => [ INNER_FLAG, ... ], ... ]
>
> you'd get "must not set this flag" with the error offset pointing to
> that; if I didn't do this construction here with inner messages winning,
> you'd get "Attribute failed policy validation" with the error offset
> pointing to the "OUTER_NESTING" attribute, that's pretty useless.
Yes, agree.
>
> From an external API POV though, nla_validate/nla_parse will continue to
> unconditionally overwrite any existing "warning" messages with errors,
> if such occurred. They just won't overwrite their own messages when
> returning from a nested policy validation.
Yes, that's fine. I'm not objecting to the behavior. It just feels
that extack handling is getting tricky by the day and we could seize
the moment to improve it while we can.
Marcelo
^ permalink raw reply
* [PATCH] net-next: mscc: remove unused ocelot_dev_gmii.h
From: Corentin Labbe @ 2018-09-19 19:06 UTC (permalink / raw)
To: alexandre.belloni, davem; +Cc: linux-kernel, netdev, Corentin Labbe
The header ocelot_dev_gmii.h is unused since the inclusion of the driver.
It is unused, lets just remove it.
Signed-off-by: Corentin Labbe <clabbe@baylibre.com>
---
drivers/net/ethernet/mscc/ocelot_dev_gmii.h | 154 ----------------------------
1 file changed, 154 deletions(-)
delete mode 100644 drivers/net/ethernet/mscc/ocelot_dev_gmii.h
diff --git a/drivers/net/ethernet/mscc/ocelot_dev_gmii.h b/drivers/net/ethernet/mscc/ocelot_dev_gmii.h
deleted file mode 100644
index 6aa40ea..0000000
--- a/drivers/net/ethernet/mscc/ocelot_dev_gmii.h
+++ /dev/null
@@ -1,154 +0,0 @@
-/* SPDX-License-Identifier: (GPL-2.0 OR MIT) */
-/*
- * Microsemi Ocelot Switch driver
- *
- * Copyright (c) 2017 Microsemi Corporation
- */
-
-#ifndef _MSCC_OCELOT_DEV_GMII_H_
-#define _MSCC_OCELOT_DEV_GMII_H_
-
-#define DEV_GMII_PORT_MODE_CLOCK_CFG 0x0
-
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_MAC_TX_RST BIT(5)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_MAC_RX_RST BIT(4)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_PORT_RST BIT(3)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_PHY_RST BIT(2)
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_LINK_SPEED(x) ((x) & GENMASK(1, 0))
-#define DEV_GMII_PORT_MODE_CLOCK_CFG_LINK_SPEED_M GENMASK(1, 0)
-
-#define DEV_GMII_PORT_MODE_PORT_MISC 0x4
-
-#define DEV_GMII_PORT_MODE_PORT_MISC_MPLS_RX_ENA BIT(5)
-#define DEV_GMII_PORT_MODE_PORT_MISC_FWD_ERROR_ENA BIT(4)
-#define DEV_GMII_PORT_MODE_PORT_MISC_FWD_PAUSE_ENA BIT(3)
-#define DEV_GMII_PORT_MODE_PORT_MISC_FWD_CTRL_ENA BIT(2)
-#define DEV_GMII_PORT_MODE_PORT_MISC_GMII_LOOP_ENA BIT(1)
-#define DEV_GMII_PORT_MODE_PORT_MISC_DEV_LOOP_ENA BIT(0)
-
-#define DEV_GMII_PORT_MODE_EVENTS 0x8
-
-#define DEV_GMII_PORT_MODE_EEE_CFG 0xc
-
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_ENA BIT(22)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_AGE(x) (((x) << 15) & GENMASK(21, 15))
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_AGE_M GENMASK(21, 15)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_AGE_X(x) (((x) & GENMASK(21, 15)) >> 15)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_WAKEUP(x) (((x) << 8) & GENMASK(14, 8))
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_WAKEUP_M GENMASK(14, 8)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_WAKEUP_X(x) (((x) & GENMASK(14, 8)) >> 8)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_HOLDOFF(x) (((x) << 1) & GENMASK(7, 1))
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_HOLDOFF_M GENMASK(7, 1)
-#define DEV_GMII_PORT_MODE_EEE_CFG_EEE_TIMER_HOLDOFF_X(x) (((x) & GENMASK(7, 1)) >> 1)
-#define DEV_GMII_PORT_MODE_EEE_CFG_PORT_LPI BIT(0)
-
-#define DEV_GMII_PORT_MODE_RX_PATH_DELAY 0x10
-
-#define DEV_GMII_PORT_MODE_TX_PATH_DELAY 0x14
-
-#define DEV_GMII_PORT_MODE_PTP_PREDICT_CFG 0x18
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ENA_CFG 0x1c
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ENA_CFG_RX_ENA BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ENA_CFG_TX_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG 0x20
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG_FC_WORD_SYNC_ENA BIT(8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG_GIGA_MODE_ENA BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MODE_CFG_FDX_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_MAXLEN_CFG 0x24
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG 0x28
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_TAG_ID(x) (((x) << 16) & GENMASK(31, 16))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_TAG_ID_M GENMASK(31, 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_TAG_ID_X(x) (((x) & GENMASK(31, 16)) >> 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_PB_ENA BIT(1)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_VLAN_AWR_ENA BIT(0)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_TAGS_CFG_VLAN_LEN_AWR_ENA BIT(2)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ADV_CHK_CFG 0x2c
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_ADV_CHK_CFG_LEN_DROP_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG 0x30
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RESTORE_OLD_IPG_CHECK BIT(17)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_REDUCED_TX_IFG BIT(16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_TX_IFG(x) (((x) << 8) & GENMASK(12, 8))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_TX_IFG_M GENMASK(12, 8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_TX_IFG_X(x) (((x) & GENMASK(12, 8)) >> 8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG2(x) (((x) << 4) & GENMASK(7, 4))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG2_M GENMASK(7, 4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG2_X(x) (((x) & GENMASK(7, 4)) >> 4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG1(x) ((x) & GENMASK(3, 0))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_IFG_CFG_RX_IFG1_M GENMASK(3, 0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG 0x34
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_BYPASS_COL_SYNC BIT(26)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_OB_ENA BIT(25)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_WEXC_DIS BIT(24)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED(x) (((x) << 16) & GENMASK(23, 16))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED_M GENMASK(23, 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED_X(x) (((x) & GENMASK(23, 16)) >> 16)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_SEED_LOAD BIT(12)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_RETRY_AFTER_EXC_COL_ENA BIT(8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_LATE_COL_POS(x) ((x) & GENMASK(6, 0))
-#define DEV_GMII_MAC_CFG_STATUS_MAC_HDX_CFG_LATE_COL_POS_M GENMASK(6, 0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_DBG_CFG 0x38
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_DBG_CFG_TBI_MODE BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_DBG_CFG_IFG_CRS_EXT_CHK_ENA BIT(0)
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_FC_MAC_LOW_CFG 0x3c
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_FC_MAC_HIGH_CFG 0x40
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY 0x44
-
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_IPG_SHRINK_STICKY BIT(9)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_PREAM_SHRINK_STICKY BIT(8)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_CARRIER_EXT_STICKY BIT(7)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_CARRIER_EXT_ERR_STICKY BIT(6)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_RX_JUNK_STICKY BIT(5)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_RETRANSMIT_STICKY BIT(4)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_JAM_STICKY BIT(3)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_FIFO_OFLW_STICKY BIT(2)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_FRM_LEN_OVR_STICKY BIT(1)
-#define DEV_GMII_MAC_CFG_STATUS_MAC_STICKY_TX_ABORT_STICKY BIT(0)
-
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG 0x48
-
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG_MM_RX_ENA BIT(0)
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG_MM_TX_ENA BIT(4)
-#define DEV_GMII_MM_CONFIG_ENABLE_CONFIG_KEEP_S_AFTER_D BIT(8)
-
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG 0x4c
-
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_DIS BIT(0)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_TIME(x) (((x) << 4) & GENMASK(11, 4))
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_TIME_M GENMASK(11, 4)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_PRM_VERIFY_TIME_X(x) (((x) & GENMASK(11, 4)) >> 4)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_VERIF_TIMER_UNITS(x) (((x) << 12) & GENMASK(13, 12))
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_VERIF_TIMER_UNITS_M GENMASK(13, 12)
-#define DEV_GMII_MM_CONFIG_VERIF_CONFIG_VERIF_TIMER_UNITS_X(x) (((x) & GENMASK(13, 12)) >> 12)
-
-#define DEV_GMII_MM_STATISTICS_MM_STATUS 0x50
-
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_ACTIVE_STATUS BIT(0)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_ACTIVE_STICKY BIT(4)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_VERIFY_STATE(x) (((x) << 8) & GENMASK(10, 8))
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_VERIFY_STATE_M GENMASK(10, 8)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_PRMPT_VERIFY_STATE_X(x) (((x) & GENMASK(10, 8)) >> 8)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_UNEXP_RX_PFRM_STICKY BIT(12)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_UNEXP_TX_PFRM_STICKY BIT(16)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_MM_RX_FRAME_STATUS BIT(20)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_MM_TX_FRAME_STATUS BIT(24)
-#define DEV_GMII_MM_STATISTICS_MM_STATUS_MM_TX_PRMPT_STATUS BIT(28)
-
-#endif
--
2.7.4
^ permalink raw reply related
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: Marcelo Ricardo Leitner @ 2018-09-19 19:08 UTC (permalink / raw)
To: David Ahern; +Cc: Johannes Berg, linux-wireless, netdev
In-Reply-To: <50773483-5732-8874-c5bf-99fa09d7e94a@gmail.com>
On Wed, Sep 19, 2018 at 09:44:37AM -0700, David Ahern wrote:
> On 9/19/18 9:36 AM, Johannes Berg wrote:
> > On Wed, 2018-09-19 at 09:28 -0700, David Ahern wrote:
> >> On 9/19/18 5:08 AM, Johannes Berg wrote:
> >>> diff --git a/lib/nlattr.c b/lib/nlattr.c
> >>> index 966cd3dcf31b..2b015e43b725 100644
> >>> --- a/lib/nlattr.c
> >>> +++ b/lib/nlattr.c
> >>> @@ -69,7 +69,7 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
> >>>
> >>> static int validate_nla(const struct nlattr *nla, int maxtype,
> >>> const struct nla_policy *policy,
> >>> - const char **error_msg)
> >>> + struct netlink_ext_ack *extack, bool *extack_set)
> >>
> >> extack_set arg is not needed if you handle the "Attribute failed policy
> >> validation" message and NL_SET_BAD_ATTR here as well.
> >
> > I'm not sure that's true, but perhaps you have a better idea than me?
> >
> > My thought would be to introduce an "error" label in validate_nla(),
> > that sets up the extack data.
> >
> > Then we could skip over that if we have a separate message to report,
> > making the NLA_REJECT case easier.
> >
> > However, if we do nested validation, I'm not sure it really is that much
> > easier? We still need to figure out if the nested validation was setting
> > the message (and bad attr), rather than it having been set before we
> > even get into this function.
> >
> > So let's say we have
> >
> > case NLA_NESTED:
> > /* a nested attributes is allowed to be empty; if its not,
> > * it must have a size of at least NLA_HDRLEN.
> > */
> > if (attrlen == 0)
> > break;
> > if (attrlen < NLA_HDRLEN)
> > return -ERANGE;
> > if (pt->validation_data) {
> > int err;
> >
> > err = nla_validate_parse(nla_data(nla), nla_len(nla),
> > pt->len, pt->validation_data,
> > extack, extack_set, NULL);
> > if (err < 0)
> > return err;
> > }
> > break;
> >
> > right now after all the patches.
> >
> > The "return -ERANGE;" would become "{ err = -ERANGE; goto error; }", but
> > I'm not really sure we can cleanly handle the other case?
> >
> > Hmm. Maybe it works if we ensure that nla_validate_parse() has no other
> > return points that can fail outside of validate_nla(), or we set up the
> > extack data there as well, so that once we have a nested
> > nla_validate_parse() we know that it's been set.
> >
> > Actually, we need to do that anyway so that we can move the setting into
> > validate_nla(), and then it should work.
> >
> > Mechanics aside - I'll take a look later tonight or tomorrow - do you
> > think the goal/external interface of this makes sense?
>
> If it fails and returns (nested and all) on the first failure it should
> be fine. I was thinking something like this (whitespace damaged on paste):
This will avoid the situation that we were discussing in the older
thread, btw.
Marcelo
^ permalink raw reply
* Re: [PATCH 5/7] netlink: prepare validate extack setting for recursion
From: Johannes Berg @ 2018-09-19 19:09 UTC (permalink / raw)
To: Marcelo Ricardo Leitner, David Ahern; +Cc: linux-wireless, netdev
In-Reply-To: <20180919190851.GM4590@localhost.localdomain>
On Wed, 2018-09-19 at 16:08 -0300, Marcelo Ricardo Leitner wrote:
>
> > If it fails and returns (nested and all) on the first failure it should
> > be fine. I was thinking something like this (whitespace damaged on paste):
>
> This will avoid the situation that we were discussing in the older
> thread, btw.
I think it only avoids the part where we have to worry about "have I
already set this" - which is David's point AFAICT.
I'll reply over to your other email (as I already started writing a
reply there)
johannes
^ permalink raw reply
* [PATCH] mpls: allow routes on ip6gre devices
From: Saif Hhasan @ 2018-09-19 17:15 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Calvin Owens, Saif Hasan
From: Saif Hasan <has@fb.com>
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH] mpls: allow routes on ip6gre devices
From: Saif Hasan @ 2018-09-19 17:20 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Calvin Owens, Saif Hasan
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH] mpls: allow routes on ip6gre devices
From: Saif Hasan @ 2018-09-19 19:17 UTC (permalink / raw)
To: netdev; +Cc: David S . Miller, Calvin Owens, Saif Hasan, Saif Hasan
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* Re: [RFC 4/5] netlink: prepare validate extack setting for recursion
From: Johannes Berg @ 2018-09-19 19:19 UTC (permalink / raw)
To: Marcelo Ricardo Leitner; +Cc: netdev
In-Reply-To: <20180919184652.GL4590@localhost.localdomain>
On Wed, 2018-09-19 at 15:46 -0300, Marcelo Ricardo Leitner wrote:
> > NL_SET_ERR_MSG(extack, "warning: deprecated command");
> > err = nla_parse(..., extack);
> > if (err)
> > return err;
> > /* do something */
> > return 0;
> >
> > Here you could consider the message there a warning that's transported
> > out even if we return 0, but if we return with a failure from
> > nla_parse() (or nla_validate instead if you wish), then that failure
> > message "wins".
>
> Agree. This is the core issue here, IMHO. Once out of the context that
> set the message, we have no way of knowing if we can nor should
> overwrite the message that is already in there.
True.
I'm not really sure I'd go as far as calling it an issue though.
IMHO, we really get into this situation if the code is badly structured
to start with. Taking the above code, if you write it as
err = nla_parse(...);
if (err)
return err;
/* do something */
NLA_SET_ERR_MSG(extack, "warning: deprecated command");
return 0;
instead, then you have no such problems.
Well, perhaps you do, if "/* do something */" might *also* set the
message, but basically you have to statically decide which one wins by
ordering the code correctly.
I'm still not convinced, btw, that we actually have any instance in the
kernel today of the issue we've been discussing - namely that some code
does something like the original quoted code at the top of the email.
> > I suppose we could - technically - make that generic, in that we could
> > have both
> >
> > NLA_SET_WARN_MSG(extack, "...");
> > NLA_SET_ERR_MSG(extack, "...");
>
> I like this.
I'm not really sure what for though :-)
FWIW, if you do think that there's a need for distinguishing this, then
I'd argue that perhaps the right way to address this would be to extend
this all the way to userspace and have two separate attributes for
errors and warnings in the extended ACK message?
> > and keep track of warning vs. error; however, just like my first version
> > of the NLA_REJECT patch, that would break existing code.
>
> Hm, I may have missed something but I think the discussion in there
> was for a different context. For an extack msg to be set by
> when validate_nla() call returns on nla_parse(), the previous message
> had to be a "warning" because otherwise the parsing wouldn't be even
> attempted. So in that case, we are safe to simply overwrite it.
Yes, arguably, if you really had an "error" then you'd probably have
never gotten to the parsing. It's possible - although sort of stupid -
to write this code too though:
NL_SET_ERR_MSG(extack, "error: unsupported command");
err = nla_parse(...);
return err ? : -EOPNOTSUPP;
Which one should win in that case?
Again, in my opinion we've got enough flexibility if we let
nla_parse/nla_validate behave as today (mostly, nla_validate doesn't set
a message and I'd like to change that, it does set the error attribute
pointer/offset) and let the calling code sort it out.
In many cases nla_parse() will be called by generic code (e.g.
genetlink) and you'd never get into this situation. Once you're past
that, you can do whatever you like.
> While for the situation you are describing here, it will set a generic
> error message in case the inner code didn't do it.
Yes, but that's not a change in semantics as far as the caller of
nla_parse/nla_validate is concerned - it'll continue to unconditionally
set a message if an error occurred, only the internal behaviour as to
which message seems more relevant is at issue, and the whole recursion
thing and avoiding an outer one overwriting an inner one is IMHO more
useful because that's the more specific error.
> Using the semantics of NLA_SET_WARN_MSG and ERR, then WARN would
> replace other WARNs but not ERRs, and ERR would replace other WARNs
> too but not other ERRs. All we need to do handle this is a bit in
> extack saying if the message is considered a warning or not, or an
> error/fatal message or not.
I'm still not really sure what the use case for a warning is, so not
sure I can really comment on this.
> Okay but we have split parsing of netlink messages and this could be
> useful in there too:
> In cls_api.c, tc_new_tfilter() calls nmmsg_parse() and do some
> processing, and then handle it to a classifier. cls_flower, for
> example, will then do another parsing. If, for whatever reason, flower
> failed and did not set an extack msg, tc_new_tfilter() could set a
> default error message, but at this moment we can't tell if the msg
> already set was just a warning from the first parsing (or any other
> code before engaging flower) (which we can overwrite) or if it a
> message that flower set (which we should not overwrite).
>
> Hopefully my description clear.. 8-)
>
> I think this is the same situation as with the nested parsing you're
> proposing.
Yes, I admit that's the same, just not part of pure policy checking, but
open-coded (in a way).
> Currently it (tc_new_tfilter) doesn't set any default error message,
> so this issue is/was not noticed.
True.
Except that I'd still say that tc_new_tfilter() can very well assume
that nothing has set a message before it ran, it's invoked directly by
the core netlink code after all.
So IMHO we don't have an issue here because there aren't arbitrary
callers of this and it can't know what the state is; it does in fact
know very well what the state is when it's called.
With nla_validate/nla_parse that have far more callers we can't be as
certain, although again - I doubt we actually have places today that
would run into this situation (given how all this stuff is still fairly
new).
> > From an external API POV though, nla_validate/nla_parse will continue to
> > unconditionally overwrite any existing "warning" messages with errors,
> > if such occurred. They just won't overwrite their own messages when
> > returning from a nested policy validation.
>
> Yes, that's fine. I'm not objecting to the behavior. It just feels
> that extack handling is getting tricky by the day and we could seize
> the moment to improve it while we can.
Ok, I guess that's fair. I don't think this actually changes the
"trickiness" of extack handling though; it's purely an internal detail
of nla_validate/nla_parse due to me wanting to add recursion; from an
external POV nothing changed.
johannes
^ permalink raw reply
* [PATCH v2 0/5] netlink: nested policy validation
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern
Ok, I should've tried the idea that David came up with first - it does
in fact make the code quite a bit simpler, and indeed removes the need
for the previously introduced "**error_msg" argument that I hadn't
really liked anyway.
So, changes here are:
* move setting the bad attr pointer/message into validate_nla()
* remove the recursion patch since that's no longer needed
* simply skip the generic bad attr pointer/message setting in
case of nested nla_validate() failing since that could fail
only due to validate_nla() failing inside, which already sets
the extack information
johannes
^ permalink raw reply
* [PATCH v2 3/5] netlink: move extack setting into validate_nla()
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern, Johannes Berg
In-Reply-To: <20180919194905.16462-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
This unifies the code between nla_parse() which sets the bad
attribute pointer and an error message, and nla_validate()
which only sets the bad attribute pointer.
It also cleans up the code for NLA_REJECT and paves the way
for nested policy validation, as it will allow us to easily
skip setting the "generic" message without any extra args
like the **error_msg now, just passing the extack through is
now enough.
While at it, remove the unnecessary label in nla_parse().
Suggested-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
lib/nlattr.c | 63 ++++++++++++++++++++++++++++++------------------------------
1 file changed, 32 insertions(+), 31 deletions(-)
diff --git a/lib/nlattr.c b/lib/nlattr.c
index e2e5b38394d5..ff6d48d7c19a 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -69,10 +69,11 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
- const char **error_msg)
+ struct netlink_ext_ack *extack)
{
const struct nla_policy *pt;
int minlen = 0, attrlen = nla_len(nla), type = nla_type(nla);
+ int err = -ERANGE;
if (type <= 0 || type > maxtype)
return 0;
@@ -90,24 +91,28 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
switch (pt->type) {
case NLA_EXACT_LEN:
if (attrlen != pt->len)
- return -ERANGE;
+ goto out_err;
break;
case NLA_REJECT:
- if (pt->validation_data && error_msg)
- *error_msg = pt->validation_data;
+ NL_SET_BAD_ATTR(extack, nla);
+ if (extack && pt->validation_data)
+ extack->_msg = pt->validation_data;
return -EINVAL;
case NLA_FLAG:
if (attrlen > 0)
- return -ERANGE;
+ goto out_err;
break;
case NLA_BITFIELD32:
if (attrlen != sizeof(struct nla_bitfield32))
- return -ERANGE;
+ goto out_err;
- return validate_nla_bitfield32(nla, pt->validation_data);
+ err = validate_nla_bitfield32(nla, pt->validation_data);
+ if (err)
+ goto out_err;
+ break;
case NLA_NUL_STRING:
if (pt->len)
@@ -115,13 +120,15 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
else
minlen = attrlen;
- if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL)
- return -EINVAL;
+ if (!minlen || memchr(nla_data(nla), '\0', minlen) == NULL) {
+ err = -EINVAL;
+ goto out_err;
+ }
/* fall through */
case NLA_STRING:
if (attrlen < 1)
- return -ERANGE;
+ goto out_err;
if (pt->len) {
char *buf = nla_data(nla);
@@ -130,13 +137,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
attrlen--;
if (attrlen > pt->len)
- return -ERANGE;
+ goto out_err;
}
break;
case NLA_BINARY:
if (pt->len && attrlen > pt->len)
- return -ERANGE;
+ goto out_err;
break;
case NLA_NESTED:
@@ -152,10 +159,13 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
minlen = nla_attr_minlen[pt->type];
if (attrlen < minlen)
- return -ERANGE;
+ goto out_err;
}
return 0;
+out_err:
+ NL_SET_ERR_MSG_ATTR(extack, nla, "Attribute failed policy validation");
+ return err;
}
/**
@@ -180,12 +190,10 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
int rem;
nla_for_each_attr(nla, head, len, rem) {
- int err = validate_nla(nla, maxtype, policy, NULL);
+ int err = validate_nla(nla, maxtype, policy, extack);
- if (err < 0) {
- NL_SET_BAD_ATTR(extack, nla);
+ if (err < 0)
return err;
- }
}
return 0;
@@ -241,7 +249,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
struct netlink_ext_ack *extack)
{
const struct nlattr *nla;
- int rem, err;
+ int rem;
memset(tb, 0, sizeof(struct nlattr *) * (maxtype + 1));
@@ -249,17 +257,12 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
u16 type = nla_type(nla);
if (type > 0 && type <= maxtype) {
- static const char _msg[] = "Attribute failed policy validation";
- const char *msg = _msg;
-
if (policy) {
- err = validate_nla(nla, maxtype, policy, &msg);
- if (err < 0) {
- NL_SET_BAD_ATTR(extack, nla);
- if (extack)
- extack->_msg = msg;
- goto errout;
- }
+ int err = validate_nla(nla, maxtype, policy,
+ extack);
+
+ if (err < 0)
+ return err;
}
tb[type] = (struct nlattr *)nla;
@@ -270,9 +273,7 @@ int nla_parse(struct nlattr **tb, int maxtype, const struct nlattr *head,
pr_warn_ratelimited("netlink: %d bytes leftover after parsing attributes in process `%s'.\n",
rem, current->comm);
- err = 0;
-errout:
- return err;
+ return 0;
}
EXPORT_SYMBOL(nla_parse);
--
2.14.4
^ permalink raw reply related
* [PATCH v2 5/5] netlink: add nested array policy validation
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern, Johannes Berg
In-Reply-To: <20180919194905.16462-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
Sometimes nested netlink attributes are just used as arrays, with
the nla_type() of each not being used; we have this in nl80211 and
e.g. NFTA_SET_ELEM_LIST_ELEMENTS.
Add the ability to validate this type of message directly in the
policy, by adding the type NLA_NESTED_ARRAY which does exactly
this: require a first level of nesting but ignore the attribute
type, and then inside each require a second level of nested and
validate those attributes against a given policy (if present).
Note that some nested array types actually require that all of
the entries have the same index, this is possible to express in
a nested policy already, apart from the validation that only the
one allowed type is used.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
include/net/netlink.h | 12 +++++++++++-
lib/nlattr.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 91907852da1c..3698ca8ff92c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -172,6 +172,7 @@ enum {
NLA_FLAG,
NLA_MSECS,
NLA_NESTED,
+ NLA_NESTED_ARRAY,
NLA_NUL_STRING,
NLA_BINARY,
NLA_S8,
@@ -200,7 +201,8 @@ enum {
* NLA_NUL_STRING Maximum length of string (excluding NUL)
* NLA_FLAG Unused
* NLA_BINARY Maximum length of attribute payload
- * NLA_NESTED Length verification is done by checking len of
+ * NLA_NESTED,
+ * NLA_NESTED_ARRAY Length verification is done by checking len of
* nested header (or empty); len field is used if
* validation_data is also used, for the max attr
* number in the nested policy.
@@ -230,6 +232,12 @@ enum {
* `len' to the max attribute number.
* Note that nla_parse() will validate, but of course not
* parse, the nested sub-policies.
+ * NLA_NESTED_ARRAY Points to a nested policy to validate, must also set
+ * `len' to the max attribute number. The difference to
+ * NLA_NESTED is the structure - NLA_NESTED has the
+ * nested attributes directly inside, while an array has
+ * the nested attributes at another level down and the
+ * attributes directly in the nesting don't matter.
* All other Unused
*
* Example:
@@ -255,6 +263,8 @@ struct nla_policy {
#define NLA_POLICY_NESTED(maxattr, policy) \
{ .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+#define NLA_POLICY_NESTED_ARRAY(maxattr, policy) \
+ { .type = NLA_NESTED_ARRAY, .validation_data = policy, .len = maxattr }
/**
* struct nl_info - netlink source information
diff --git a/lib/nlattr.c b/lib/nlattr.c
index 4cbfc64f6d10..9d8e1dc71680 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -67,6 +67,34 @@ static int validate_nla_bitfield32(const struct nlattr *nla,
return 0;
}
+static int nla_validate_array(const struct nlattr *head, int len, int maxtype,
+ const struct nla_policy *policy,
+ struct netlink_ext_ack *extack)
+{
+ const struct nlattr *entry;
+ int rem;
+
+ nla_for_each_attr(entry, head, len, rem) {
+ int ret;
+
+ if (nla_len(entry) == 0)
+ continue;
+
+ if (nla_len(entry) < NLA_HDRLEN) {
+ NL_SET_ERR_MSG_ATTR(extack, entry,
+ "Array element too short");
+ return -ERANGE;
+ }
+
+ ret = nla_validate(nla_data(entry), nla_len(entry),
+ maxtype, policy, extack);
+ if (ret < 0)
+ return ret;
+ }
+
+ return 0;
+}
+
static int validate_nla(const struct nlattr *nla, int maxtype,
const struct nla_policy *policy,
struct netlink_ext_ack *extack)
@@ -166,6 +194,29 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
}
}
break;
+ case NLA_NESTED_ARRAY:
+ /* a nested array attribute is allowed to be empty; if its not,
+ * it must have a size of at least NLA_HDRLEN.
+ */
+ if (attrlen == 0)
+ break;
+ if (attrlen < NLA_HDRLEN)
+ goto out_err;
+ if (pt->validation_data) {
+ int err;
+
+ err = nla_validate_array(nla_data(nla), nla_len(nla),
+ pt->len, pt->validation_data,
+ extack);
+ if (err < 0) {
+ /*
+ * return directly to preserve the inner
+ * error message/attribute pointer
+ */
+ return err;
+ }
+ }
+ break;
default:
if (pt->len)
minlen = pt->len;
--
2.14.4
^ permalink raw reply related
* [PATCH v2 4/5] netlink: allow NLA_NESTED to specify nested policy to validate
From: Johannes Berg @ 2018-09-19 19:49 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern, Johannes Berg
In-Reply-To: <20180919194905.16462-1-johannes@sipsolutions.net>
From: Johannes Berg <johannes.berg@intel.com>
Now that we have a validation_data pointer, and the len field in
the policy is unused for NLA_NESTED, we can allow using them both
to have nested validation. This can be nice in code, although we
still have to use nla_parse_nested() or similar which would also
take a policy; however, it also serves as documentation in the
policy without requiring a look at the code.
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
include/net/netlink.h | 13 +++++++++++--
lib/nlattr.c | 14 ++++++++++++++
2 files changed, 25 insertions(+), 2 deletions(-)
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 0d698215d4d9..91907852da1c 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -200,8 +200,10 @@ enum {
* NLA_NUL_STRING Maximum length of string (excluding NUL)
* NLA_FLAG Unused
* NLA_BINARY Maximum length of attribute payload
- * NLA_NESTED Don't use `len' field -- length verification is
- * done by checking len of nested header (or empty)
+ * NLA_NESTED Length verification is done by checking len of
+ * nested header (or empty); len field is used if
+ * validation_data is also used, for the max attr
+ * number in the nested policy.
* NLA_U8, NLA_U16,
* NLA_U32, NLA_U64,
* NLA_S8, NLA_S16,
@@ -224,6 +226,10 @@ enum {
* NLA_REJECT This attribute is always rejected and validation data
* may point to a string to report as the error instead
* of the generic one in extended ACK.
+ * NLA_NESTED Points to a nested policy to validate, must also set
+ * `len' to the max attribute number.
+ * Note that nla_parse() will validate, but of course not
+ * parse, the nested sub-policies.
* All other Unused
*
* Example:
@@ -247,6 +253,9 @@ struct nla_policy {
#define NLA_POLICY_ETH_ADDR NLA_POLICY_EXACT_LEN(ETH_ALEN)
#define NLA_POLICY_ETH_ADDR_COMPAT NLA_POLICY_EXACT_LEN_WARN(ETH_ALEN)
+#define NLA_POLICY_NESTED(maxattr, policy) \
+ { .type = NLA_NESTED, .validation_data = policy, .len = maxattr }
+
/**
* struct nl_info - netlink source information
* @nlh: Netlink message header of original request
diff --git a/lib/nlattr.c b/lib/nlattr.c
index ff6d48d7c19a..4cbfc64f6d10 100644
--- a/lib/nlattr.c
+++ b/lib/nlattr.c
@@ -152,6 +152,20 @@ static int validate_nla(const struct nlattr *nla, int maxtype,
*/
if (attrlen == 0)
break;
+ if (attrlen < NLA_HDRLEN)
+ goto out_err;
+ if (pt->validation_data) {
+ err = nla_validate(nla_data(nla), nla_len(nla), pt->len,
+ pt->validation_data, extack);
+ if (err < 0) {
+ /*
+ * return directly to preserve the inner
+ * error message/attribute pointer
+ */
+ return err;
+ }
+ }
+ break;
default:
if (pt->len)
minlen = pt->len;
--
2.14.4
^ permalink raw reply related
* Re: [PATCH v2 3/5] netlink: move extack setting into validate_nla()
From: Johannes Berg @ 2018-09-19 19:52 UTC (permalink / raw)
To: linux-wireless, netdev; +Cc: David Ahern
In-Reply-To: <20180919194905.16462-4-johannes@sipsolutions.net>
On Wed, 2018-09-19 at 21:49 +0200, Johannes Berg wrote:
>
> case NLA_REJECT:
> - if (pt->validation_data && error_msg)
> - *error_msg = pt->validation_data;
> + NL_SET_BAD_ATTR(extack, nla);
> + if (extack && pt->validation_data)
> + extack->_msg = pt->validation_data;
> return -EINVAL;
Damn. This of course needs to happen only if pt->validation_data is set,
otherwise it needs to "goto out_err" to set the default message.
I'll respin another day with that fixed, and any other comments that
come in until then addressed.
Sorry for the noise :-(
johannes
^ permalink raw reply
* (unknown),
From: Saif Hasan @ 2018-09-19 19:57 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Saif Hasan, Calvin Owens
>From e4f144286efe0f298c11efe58e17b1ab91c7ee3f Mon Sep 17 00:00:00 2001
From: Saif Hasan <has@fb.com>
Date: Mon, 17 Sep 2018 16:28:54 -0700
Subject: [PATCH] mpls: allow routes on ip6gre devices
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct
notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH net] mpls: allow routes on ip6gre devices
From: Saif Hasan @ 2018-09-19 19:59 UTC (permalink / raw)
To: netdev; +Cc: David S. Miller, Calvin Owens, Saif Hasan
>From e4f144286efe0f298c11efe58e17b1ab91c7ee3f Mon Sep 17 00:00:00 2001
From: Saif Hasan <has@fb.com>
Date: Mon, 17 Sep 2018 16:28:54 -0700
Subject: [PATCH] mpls: allow routes on ip6gre devices
Summary:
This appears to be necessary and sufficient change to enable `MPLS` on
`ip6gre` tunnels (RFC4023).
This diff allows IP6GRE devices to be recognized by MPLS kernel module
and hence user can configure interface to accept packets with mpls
headers as well setup mpls routes on them.
Test Plan:
Test plan consists of multiple containers connected via GRE-V6 tunnel.
Then carrying out testing steps as below.
- Carry out necessary sysctl settings on all containers
```
sysctl -w net.mpls.platform_labels=65536
sysctl -w net.mpls.ip_ttl_propagate=1
sysctl -w net.mpls.conf.lo.input=1
```
- Establish IP6GRE tunnels
```
ip -6 tunnel add name if_1_2_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::2 key 1
ip link set dev if_1_2_1 up
sysctl -w net.mpls.conf.if_1_2_1.input=1
ip -4 addr add 169.254.0.2/31 dev if_1_2_1 scope link
ip -6 tunnel add name if_1_3_1 mode ip6gre \
local 2401:db00:21:6048:feed:0::1 \
remote 2401:db00:21:6048:feed:0::3 key 1
ip link set dev if_1_3_1 up
sysctl -w net.mpls.conf.if_1_3_1.input=1
ip -4 addr add 169.254.0.4/31 dev if_1_3_1 scope link
```
- Install MPLS encap rules on node-1 towards node-2
```
ip route add 192.168.0.11/32 nexthop encap mpls 32/64 \
via inet 169.254.0.3 dev if_1_2_1
```
- Install MPLS forwarding rules on node-2 and node-3
```
// node2
ip -f mpls route add 32 via inet 169.254.0.7 dev if_2_4_1
// node3
ip -f mpls route add 64 via inet 169.254.0.12 dev if_4_3_1
```
- Ping 192.168.0.11 (node4) from 192.168.0.1 (node1) (where routing
towards 192.168.0.1 is via IP route directly towards node1 from node4)
```
ping 192.168.0.11
```
- tcpdump on interface to capture ping packets wrapped within MPLS
header which inturn wrapped within IP6GRE header
```
16:43:41.121073 IP6
2401:db00:21:6048:feed::1 > 2401:db00:21:6048:feed::2:
DSTOPT GREv0, key=0x1, length 100:
MPLS (label 32, exp 0, ttl 255) (label 64, exp 0, [S], ttl 255)
IP 192.168.0.1 > 192.168.0.11:
ICMP echo request, id 1208, seq 45, length 64
0x0000: 6000 2cdb 006c 3c3f 2401 db00 0021 6048 `.,..l<?$....!`H
0x0010: feed 0000 0000 0001 2401 db00 0021 6048 ........$....!`H
0x0020: feed 0000 0000 0002 2f00 0401 0401 0100 ......../.......
0x0030: 2000 8847 0000 0001 0002 00ff 0004 01ff ...G............
0x0040: 4500 0054 3280 4000 ff01 c7cb c0a8 0001 E..T2.@.........
0x0050: c0a8 000b 0800 a8d7 04b8 002d 2d3c a05b ...........--<.[
0x0060: 0000 0000 bcd8 0100 0000 0000 1011 1213 ................
0x0070: 1415 1617 1819 1a1b 1c1d 1e1f 2021 2223 .............!"#
0x0080: 2425 2627 2829 2a2b 2c2d 2e2f 3031 3233 $%&'()*+,-./0123
0x0090: 3435 3637 4567
```
Signed-off-by: Saif Hasan <has@fb.com>
---
net/mpls/af_mpls.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/mpls/af_mpls.c b/net/mpls/af_mpls.c
index 7a4de6d618b1..aeb5bf2f7595 100644
--- a/net/mpls/af_mpls.c
+++ b/net/mpls/af_mpls.c
@@ -1533,10 +1533,11 @@ static int mpls_dev_notify(struct
notifier_block *this, unsigned long event,
unsigned int flags;
if (event == NETDEV_REGISTER) {
- /* For now just support Ethernet, IPGRE, SIT and IPIP devices */
+ /* For now just support Ethernet, IPGRE, IP6GRE, SIT and IPIP devices */
if (dev->type == ARPHRD_ETHER ||
dev->type == ARPHRD_LOOPBACK ||
dev->type == ARPHRD_IPGRE ||
+ dev->type == ARPHRD_IP6GRE ||
dev->type == ARPHRD_SIT ||
dev->type == ARPHRD_TUNNEL) {
mdev = mpls_add_dev(dev);
^ permalink raw reply related
* [PATCH net-next 1/2] r8169: simplify RTL8169 PHY initialization
From: Heiner Kallweit @ 2018-09-19 20:00 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers; +Cc: netdev@vger.kernel.org
PCI_LATENCY_TIMER is ignored on PCIe, therefore we have to do this
for the PCI chips (version <= 06) only. Also we can move setting
PCI_CACHE_LINE_SIZE.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index 7f4647c58..a27bf5807 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4046,16 +4046,13 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
rtl_hw_phy_config(dev);
if (tp->mac_version <= RTL_GIGA_MAC_VER_06) {
+ pci_write_config_byte(tp->pci_dev, PCI_LATENCY_TIMER, 0x40);
+ pci_write_config_byte(tp->pci_dev, PCI_CACHE_LINE_SIZE, 0x08);
netif_dbg(tp, drv, dev,
"Set MAC Reg C+CR Offset 0x82h = 0x01h\n");
RTL_W8(tp, 0x82, 0x01);
}
- pci_write_config_byte(tp->pci_dev, PCI_LATENCY_TIMER, 0x40);
-
- if (tp->mac_version <= RTL_GIGA_MAC_VER_06)
- pci_write_config_byte(tp->pci_dev, PCI_CACHE_LINE_SIZE, 0x08);
-
if (tp->mac_version == RTL_GIGA_MAC_VER_02) {
netif_dbg(tp, drv, dev,
"Set MAC Reg C+CR Offset 0x82h = 0x01h\n");
--
2.19.0
^ permalink raw reply related
* [PATCH net-next 2/2] r8169: remove duplicated RTL8169s PHY initialization steps
From: Heiner Kallweit @ 2018-09-19 20:02 UTC (permalink / raw)
To: David Miller, Realtek linux nic maintainers; +Cc: netdev@vger.kernel.org
In-Reply-To: <35548e5c-2421-04b6-b6d7-335c1b7fb62d@gmail.com>
Setting register 0x82 to value 01 is done a few lines before for all
chip versions <= 06 anyway. And setting PHY register 0x0b to value 00
is done at the end of rtl8169s_hw_phy_config() already. So we can
remove this.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/ethernet/realtek/r8169.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index a27bf5807..e2713867c 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -4053,15 +4053,6 @@ static void rtl8169_init_phy(struct net_device *dev, struct rtl8169_private *tp)
RTL_W8(tp, 0x82, 0x01);
}
- if (tp->mac_version == RTL_GIGA_MAC_VER_02) {
- netif_dbg(tp, drv, dev,
- "Set MAC Reg C+CR Offset 0x82h = 0x01h\n");
- RTL_W8(tp, 0x82, 0x01);
- netif_dbg(tp, drv, dev,
- "Set PHY Reg 0x0bh = 0x00h\n");
- rtl_writephy(tp, 0x0b, 0x0000); //w 0x0b 15 0 0
- }
-
/* We may have called phy_speed_down before */
phy_speed_up(dev->phydev);
--
2.19.0
^ permalink raw reply related
* [PATCH net] xfrm: validate template mode
From: Sean Tranchetti @ 2018-09-19 19:54 UTC (permalink / raw)
To: netdev, steffen.klassert; +Cc: Sean Tranchetti
XFRM mode parameters passed as part of the user templates
in the IP_XFRM_POLICY are never properly validated. Passing
values other than valid XFRM modes can cause stack-out-of-bounds
reads to occur later in the XFRM processing:
[ 140.535608] ================================================================
[ 140.543058] BUG: KASAN: stack-out-of-bounds in xfrm_state_find+0x17e4/0x1cc4
[ 140.550306] Read of size 4 at addr ffffffc0238a7a58 by task repro/5148
[ 140.557369]
[ 140.558927] Call trace:
[ 140.558936] dump_backtrace+0x0/0x388
[ 140.558940] show_stack+0x24/0x30
[ 140.558946] __dump_stack+0x24/0x2c
[ 140.558949] dump_stack+0x8c/0xd0
[ 140.558956] print_address_description+0x74/0x234
[ 140.558960] kasan_report+0x240/0x264
[ 140.558963] __asan_report_load4_noabort+0x2c/0x38
[ 140.558967] xfrm_state_find+0x17e4/0x1cc4
[ 140.558971] xfrm_resolve_and_create_bundle+0x40c/0x1fb8
[ 140.558975] xfrm_lookup+0x238/0x1444
[ 140.558977] xfrm_lookup_route+0x48/0x11c
[ 140.558984] ip_route_output_flow+0x88/0xc4
[ 140.558991] raw_sendmsg+0xa74/0x266c
[ 140.558996] inet_sendmsg+0x258/0x3b0
[ 140.559002] sock_sendmsg+0xbc/0xec
[ 140.559005] SyS_sendto+0x3a8/0x5a8
[ 140.559008] el0_svc_naked+0x34/0x38
[ 140.559009]
[ 140.592245] page dumped because: kasan: bad access detected
[ 140.597981] page_owner info is not active (free page?)
[ 140.603267]
[ 140.653503] ================================================================
Signed-off-by: Sean Tranchetti <stranche@codeaurora.org>
---
net/xfrm/xfrm_user.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 4791aa8..ab2f280 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1480,6 +1480,9 @@ static int validate_tmpl(int nr, struct xfrm_user_tmpl *ut, u16 family)
(ut[i].family != prev_family))
return -EINVAL;
+ if (ut[i].mode >= XFRM_MODE_MAX)
+ return -EINVAL;
+
prev_family = ut[i].family;
switch (ut[i].family) {
--
1.9.1
^ permalink raw reply related
* [PATCH] net: toshiba: remove a redundant local variable 'index_specified'
From: zhong jiang @ 2018-09-20 1:56 UTC (permalink / raw)
To: davem; +Cc: benh, paulus, mpe, netdev, linux-kernel
The local variable 'index_specified' is never used after being assigned.
hence it should be redundant adn can be removed.
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
drivers/net/ethernet/toshiba/ps3_gelic_wireless.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
index 302079e..00ab417 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
@@ -1094,7 +1094,7 @@ static int gelic_wl_get_encode(struct net_device *netdev,
struct gelic_wl_info *wl = port_wl(netdev_priv(netdev));
struct iw_point *enc = &data->encoding;
unsigned long irqflag;
- unsigned int key_index, index_specified;
+ unsigned int key_index;
int ret = 0;
pr_debug("%s: <-\n", __func__);
@@ -1105,13 +1105,10 @@ static int gelic_wl_get_encode(struct net_device *netdev,
return -EINVAL;
spin_lock_irqsave(&wl->lock, irqflag);
- if (key_index) {
- index_specified = 1;
+ if (key_index)
key_index--;
- } else {
- index_specified = 0;
+ else
key_index = wl->current_key;
- }
if (wl->group_cipher_method == GELIC_WL_CIPHER_WEP) {
switch (wl->auth_method) {
--
1.7.12.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