lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM0EoM=ZKcJXN4Y-wr0ip=eVkbRNv95SbYG5TQamFDimxWsG1Q@mail.gmail.com>
Date:   Thu, 26 Jan 2023 09:54:19 -0500
From:   Jamal Hadi Salim <jhs@...atatu.com>
To:     Vlad Buslov <vladbu@...dia.com>
Cc:     netdev@...r.kernel.org, kernel@...atatu.com,
        deb.chatterjee@...el.com, anjali.singhai@...el.com,
        namrata.limaye@...el.com, khalidm@...dia.com, tom@...anda.io,
        pratyush@...anda.io, jiri@...nulli.us, xiyou.wangcong@...il.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, simon.horman@...igine.com
Subject: Re: [PATCH net-next RFC 14/20] p4tc: add header field create, get,
 delete, flush and dump

On Wed, Jan 25, 2023 at 4:44 PM Vlad Buslov <vladbu@...dia.com> wrote:
>

[..]

> > +                                    struct netlink_ext_ack *extack)
> > +{
> > +     struct p4tc_hdrfield *hdrfield;
> > +
> > +     hdrfield = tcf_hdrfield_find_byany(parser, hdrfield_name, hdrfield_id,
> > +                                        extack);
> > +     if (IS_ERR(hdrfield))
> > +             return hdrfield;
> > +
> > +     /* Should never happen */
> > +     WARN_ON(!refcount_inc_not_zero(&hdrfield->hdrfield_ref));
>
> I think regular refcount_inc() already generates a warning when
> reference value is 0.

The thought here was we wanted to ensure ordering and i think somewhere
(maybe in the kernel doc?) it says refcount_inc_not_zero() ensures memory
ordering with dec(). This should only be needed if there is datapath
interaction with headers - i think there's none. We will review. i.e rtnl_lock
protection may be sufficient.

> > +
> > +     return hdrfield;
> > +}
> > +
> > +void tcf_hdrfield_put_ref(struct p4tc_hdrfield *hdrfield)
> > +{
> > +     WARN_ON(!refcount_dec_not_one(&hdrfield->hdrfield_ref));
>
> ditto
>
> > +}
> > +


cheers,
jamal
> > + * Copyright (c) 2022, Mojatatu Networks
> > + * Copyright (c) 2022, Intel Corporation.
> > + * Authors:     Jamal Hadi Salim <jhs@...atatu.com>
> > + *              Victor Nogueira <victor@...atatu.com>
> > + *              Pedro Tammela <pctammela@...atatu.com>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +#include <linux/errno.h>
> > +#include <linux/slab.h>
> > +#include <linux/skbuff.h>
> > +#include <linux/err.h>
> > +#include <linux/module.h>
> > +#include <net/net_namespace.h>
> > +#include <net/pkt_cls.h>
> > +#include <net/p4tc.h>
> > +#include <net/kparser.h>
> > +#include <net/netlink.h>
> > +
> > +static struct p4tc_parser *parser_find_name(struct p4tc_pipeline *pipeline,
> > +                                         const char *parser_name)
> > +{
> > +     if (unlikely(!pipeline->parser))
> > +             return NULL;
> > +
> > +     if (!strncmp(pipeline->parser->parser_name, parser_name, PARSERNAMSIZ))
> > +             return pipeline->parser;
> > +
> > +     return NULL;
> > +}
> > +
> > +struct p4tc_parser *tcf_parser_find_byid(struct p4tc_pipeline *pipeline,
> > +                                      const u32 parser_inst_id)
> > +{
> > +     if (unlikely(!pipeline->parser))
> > +             return NULL;
> > +
> > +     if (parser_inst_id == pipeline->parser->parser_inst_id)
> > +             return pipeline->parser;
> > +
> > +     return NULL;
> > +}
> > +
> > +static struct p4tc_parser *__parser_find(struct p4tc_pipeline *pipeline,
> > +                                      const char *parser_name,
> > +                                      u32 parser_inst_id,
> > +                                      struct netlink_ext_ack *extack)
> > +{
> > +     struct p4tc_parser *parser;
> > +     int err;
> > +
> > +     if (parser_inst_id) {
> > +             parser = tcf_parser_find_byid(pipeline, parser_inst_id);
> > +             if (!parser) {
> > +                     if (extack)
> > +                             NL_SET_ERR_MSG(extack,
> > +                                            "Unable to find parser by id");
> > +                     err = -EINVAL;
> > +                     goto out;
> > +             }
> > +     } else {
> > +             if (parser_name) {
> > +                     parser = parser_find_name(pipeline, parser_name);
> > +                     if (!parser) {
> > +                             if (extack)
> > +                                     NL_SET_ERR_MSG(extack,
> > +                                                    "Parser name not found");
> > +                             err = -EINVAL;
> > +                             goto out;
> > +                     }
> > +             } else {
> > +                     if (extack)
> > +                             NL_SET_ERR_MSG(extack,
> > +                                            "Must specify parser name or id");
> > +                     err = -EINVAL;
> > +                     goto out;
> > +             }
> > +     }
> > +
> > +     return parser;
> > +
> > +out:
> > +     return ERR_PTR(err);
> > +}
> > +
> > +struct p4tc_parser *tcf_parser_find_byany(struct p4tc_pipeline *pipeline,
> > +                                       const char *parser_name,
> > +                                       u32 parser_inst_id,
> > +                                       struct netlink_ext_ack *extack)
> > +{
> > +     return __parser_find(pipeline, parser_name, parser_inst_id, extack);
> > +}
> > +
> > +#ifdef CONFIG_KPARSER
> > +int tcf_skb_parse(struct sk_buff *skb, struct p4tc_skb_ext *p4tc_skb_ext,
> > +               struct p4tc_parser *parser)
> > +{
> > +     void *hdr = skb_mac_header(skb);
> > +     size_t pktlen = skb_mac_header_len(skb) + skb->len;
> > +
> > +     return __kparser_parse(parser->kparser, hdr, pktlen,
> > +                            p4tc_skb_ext->p4tc_ext->hdrs, HEADER_MAX_LEN);
> > +}
> > +
> > +static int __tcf_parser_fill(struct p4tc_parser *parser,
> > +                          struct netlink_ext_ack *extack)
> > +{
> > +     struct kparser_hkey kparser_key = { 0 };
> > +
> > +     kparser_key.id = parser->parser_inst_id;
> > +     strscpy(kparser_key.name, parser->parser_name, KPARSER_MAX_NAME);
> > +
> > +     parser->kparser = kparser_get_parser(&kparser_key, false);
> > +     if (!parser->kparser) {
> > +             NL_SET_ERR_MSG(extack, "Unable to get kparser instance");
> > +             return -ENOENT;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +void __tcf_parser_put(struct p4tc_parser *parser)
> > +{
> > +     kparser_put_parser(parser->kparser, false);
> > +}
> > +
> > +bool tcf_parser_is_callable(struct p4tc_parser *parser)
> > +{
> > +     return parser && parser->kparser;
> > +}
> > +#else
> > +int tcf_skb_parse(struct sk_buff *skb, struct p4tc_skb_ext *p4tc_skb_ext,
> > +               struct p4tc_parser *parser)
> > +{
> > +     return 0;
> > +}
> > +
> > +static int __tcf_parser_fill(struct p4tc_parser *parser,
> > +                          struct netlink_ext_ack *extack)
> > +{
> > +     return 0;
> > +}
> > +
> > +void __tcf_parser_put(struct p4tc_parser *parser)
> > +{
> > +}
> > +
> > +bool tcf_parser_is_callable(struct p4tc_parser *parser)
> > +{
> > +     return !!parser;
> > +}
> > +#endif
> > +
> > +struct p4tc_parser *
> > +tcf_parser_create(struct p4tc_pipeline *pipeline, const char *parser_name,
> > +               u32 parser_inst_id, struct netlink_ext_ack *extack)
> > +{
> > +     struct p4tc_parser *parser;
> > +     int ret;
> > +
> > +     if (pipeline->parser) {
> > +             NL_SET_ERR_MSG(extack,
> > +                            "Can only have one parser instance per pipeline");
> > +             return ERR_PTR(-EEXIST);
> > +     }
> > +
> > +     parser = kzalloc(sizeof(*parser), GFP_KERNEL);
> > +     if (!parser)
> > +             return ERR_PTR(-ENOMEM);
> > +
> > +     if (parser_inst_id)
> > +             parser->parser_inst_id = parser_inst_id;
> > +     else
> > +             /* Assign to KPARSER_KMOD_ID_MAX + 1 if no ID was supplied */
> > +             parser->parser_inst_id = KPARSER_KMOD_ID_MAX + 1;
> > +
> > +     strscpy(parser->parser_name, parser_name, PARSERNAMSIZ);
> > +
> > +     ret = __tcf_parser_fill(parser, extack);
> > +     if (ret < 0)
> > +             goto err;
> > +
> > +     refcount_set(&parser->parser_ref, 1);
> > +
> > +     idr_init(&parser->hdr_fields_idr);
> > +
> > +     pipeline->parser = parser;
> > +
> > +     return parser;
> > +
> > +err:
> > +     kfree(parser);
> > +     return ERR_PTR(ret);
> > +}
> > +
> > +/* Dummy function which just returns true
> > + * Once we have the proper parser code, this function will work properly
> > + */
> > +bool tcf_parser_check_hdrfields(struct p4tc_parser *parser,
> > +                             struct p4tc_hdrfield *hdrfield)
> > +{
> > +     return true;
> > +}
> > +
> > +int tcf_parser_del(struct net *net, struct p4tc_pipeline *pipeline,
> > +                struct p4tc_parser *parser, struct netlink_ext_ack *extack)
> > +{
> > +     struct p4tc_hdrfield *hdrfield;
> > +     unsigned long hdr_field_id, tmp;
> > +
> > +     __tcf_parser_put(parser);
> > +
> > +     idr_for_each_entry_ul(&parser->hdr_fields_idr, hdrfield, tmp, hdr_field_id)
> > +             hdrfield->common.ops->put(net, &hdrfield->common, true, extack);
> > +
> > +     idr_destroy(&parser->hdr_fields_idr);
> > +
> > +     pipeline->parser = NULL;
> > +
> > +     kfree(parser);
> > +
> > +     return 0;
> > +}
> > diff --git a/net/sched/p4tc/p4tc_pipeline.c b/net/sched/p4tc/p4tc_pipeline.c
> > index 49f0062ad..6fc7bd49d 100644
> > --- a/net/sched/p4tc/p4tc_pipeline.c
> > +++ b/net/sched/p4tc/p4tc_pipeline.c
> > @@ -115,6 +115,8 @@ static int tcf_pipeline_put(struct net *net,
> >          }
> >
> >       idr_remove(&pipe_net->pipeline_idr, pipeline->common.p_id);
> > +     if (pipeline->parser)
> > +             tcf_parser_del(net, pipeline, pipeline->parser, extack);
> >
> >       idr_for_each_entry_ul(&pipeline->p_meta_idr, meta, tmp, m_id)
> >               meta->common.ops->put(net, &meta->common, true, extack);
> > @@ -319,6 +321,8 @@ static struct p4tc_pipeline *tcf_pipeline_create(struct net *net,
> >               pipeline->num_postacts = 0;
> >       }
> >
> > +     pipeline->parser = NULL;
> > +
> >       idr_init(&pipeline->p_meta_idr);
> >       pipeline->p_meta_offset = 0;
> >
> > diff --git a/net/sched/p4tc/p4tc_tmpl_api.c b/net/sched/p4tc/p4tc_tmpl_api.c
> > index a13d02ce5..325b56d2e 100644
> > --- a/net/sched/p4tc/p4tc_tmpl_api.c
> > +++ b/net/sched/p4tc/p4tc_tmpl_api.c
> > @@ -43,6 +43,7 @@ static bool obj_is_valid(u32 obj)
> >       switch (obj) {
> >       case P4TC_OBJ_PIPELINE:
> >       case P4TC_OBJ_META:
> > +     case P4TC_OBJ_HDR_FIELD:
> >               return true;
> >       default:
> >               return false;
> > @@ -52,6 +53,7 @@ static bool obj_is_valid(u32 obj)
> >  static const struct p4tc_template_ops *p4tc_ops[P4TC_OBJ_MAX] = {
> >       [P4TC_OBJ_PIPELINE] = &p4tc_pipeline_ops,
> >       [P4TC_OBJ_META] = &p4tc_meta_ops,
> > +     [P4TC_OBJ_HDR_FIELD] = &p4tc_hdrfield_ops,
> >  };
> >
> >  int tcf_p4_tmpl_generic_dump(struct sk_buff *skb, struct p4tc_dump_ctx *ctx,
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ