From: Jamal Hadi Salim <jhs@mojatatu•com>
To: Cong Wang <xiyou.wangcong@gmail•com>
Cc: David Miller <davem@davemloft•net>,
Linux Kernel Network Developers <netdev@vger•kernel.org>,
Daniel Borkmann <daniel@iogearbox•net>,
Alexei Starovoitov <alexei.starovoitov@gmail•com>,
john fastabend <john.fastabend@gmail•com>,
dj@verizon•com
Subject: Re: [net-next PATCH v2 1/5] introduce IFE action
Date: Wed, 24 Feb 2016 08:09:43 -0500 [thread overview]
Message-ID: <56CDAB97.1060505@mojatatu.com> (raw)
In-Reply-To: <CAM_iQpXb4=PP9=oNWH1vNHb81OGg7ivUtPaD8KfdQqVNYMKgoA@mail.gmail.com>
On 16-02-23 04:44 PM, Cong Wang wrote:
> On Tue, Feb 23, 2016 at 4:49 AM, Jamal Hadi Salim <jhs@mojatatu•com> wrote:
>> +#define MODULE_ALIAS_IFE_META(metan) MODULE_ALIAS("ifemeta" __stringify_1(metan))
>> +
>> +int get_meta_u32(struct sk_buff *skb, struct tcf_meta_info *mi);
>> +int get_meta_u16(struct sk_buff *skb, struct tcf_meta_info *mi);
>> +int tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen, const void *dval);
>> +int alloc_meta_u32(struct tcf_meta_info *mi, void *metaval);
>> +int alloc_meta_u16(struct tcf_meta_info *mi, void *metaval);
>> +int check_meta_u32(u32 metaval, struct tcf_meta_info *mi);
>> +int encode_meta_u32(u32 metaval, void *skbdata, struct tcf_meta_info *mi);
>> +int validate_meta_u32(void *val, int len);
>> +int validate_meta_u16(void *val, int len);
>> +void release_meta_gen(struct tcf_meta_info *mi);
>> +int register_ife_op(struct tcf_meta_ops *mops);
>> +int unregister_ife_op(struct tcf_meta_ops *mops);
>
>
> These are exported to other modules, please add some prefix to protect them.
>
suggestion? I thought they seemed unique enough.
>> + * copyright Jamal Hadi Salim (2015)
>
>
> 2016? ;)
>
Yes. This code has been around since then. Actually earlier than that
running. But i formatted it for kernel inclusion in about first week
of January. Dave asked me to wait for the ethertype to get it in.
The IETF still hasnt come through a year later and so i re-submitted
with no default ethertype. I think 2015 is deserving here, no?
I hope i dont spend another year discussing on the list ;-> /me runs
>
>> + return 8;
>
>
> Why 8?
>
Magic number;-> I will add a comment.
It is a TLV that includes 4 bytes. I am going to wait for
comments to settle then make an update.
>> +
>> +
>> +int alloc_meta_u32(struct tcf_meta_info *mi, void *metaval)
>> +{
>> + mi->metaval = kzalloc(sizeof(u32), GFP_KERNEL);
>> + if (!mi->metaval)
>> + return -ENOMEM;
>> +
>> + memcpy(mi->metaval, metaval, 4);
>
>
> kmemdup()?
>
Sure. I'll take care of the rest you pointed to.
>
>
> No need to initi it since list_add_tail() is just one line below.
>
>
>> + list_add_tail(&mops->list, &ifeoplist);
>> + write_unlock(&ife_mod_lock);
>> + return 0;
>> +}
>> +
>> +int unregister_ife_op(struct tcf_meta_ops *mops)
>> +{
>> + struct tcf_meta_ops *m;
>> + int err = -ENOENT;
>> +
>> + write_lock(&ife_mod_lock);
>> + list_for_each_entry(m, &ifeoplist, list) {
>> + if (m->metaid == mops->metaid) {
>> + list_del(&mops->list);
>> + err = 0;
>> + break;
>> + }
>> + }
>> + write_unlock(&ife_mod_lock);
>> +
>> + return err;
>> +}
>> +
>> +EXPORT_SYMBOL_GPL(register_ife_op);
>> +EXPORT_SYMBOL_GPL(unregister_ife_op);
>
> Move them next to their definitions.
>
>
>> +
>> +void print_ife_oplist(void)
>> +{
>> + struct tcf_meta_ops *o;
>> + int i = 0;
>> +
>> + read_lock(&ife_mod_lock);
>> + list_for_each_entry(o, &ifeoplist, list) {
>> + pr_debug("%d: %s metaid %d\n", ++i, o->name, o->metaid);
>> + }
>> + read_unlock(&ife_mod_lock);
>> +}
>> +
>> +/* called when adding new meta information
>> + * under ife->tcf_lock
>> +*/
>> +int load_metaops_and_vet(struct tcf_ife_info *ife, u32 metaid,
>> + void *val, int len)
>
>
> I failed to understand the name of this function.
>
It tries to load a metadata kernel module and vets (or validates)
the passed parameters from user space.
Suggestions?
>> +{
>> + struct tcf_meta_ops *ops = find_ife_oplist(metaid);
>> + int ret = 0;
>> +
>> + if (!ops) {
>> + ret = -ENOENT;
>> +#ifdef CONFIG_MODULES
>> + spin_unlock_bh(&ife->tcf_lock);
>> + rtnl_unlock();
>> + request_module("ifemeta%u", metaid);
>> + rtnl_lock();
>> + spin_lock_bh(&ife->tcf_lock);
>> + ops = find_ife_oplist(metaid);
>> +#endif
>> + }
>> +
>> + if (ops) {
>> + ret = 0;
>> +
>> + /* XXX: unfortunately cant use nla_policy at this point
>> + * because a length of 0 is valid in the case of
>> + * "allow". "use" semantics do enforce for proper
>> + * length and i couldve use nla_policy but it makes it hard
>> + * to use it just for that..
>> + */
>> + if (len) {
>> + if (ops->validate) {
>> + ret = ops->validate(val, len);
>> + } else {
>> + if (ops->metatype == NLA_U32) {
>> + ret = validate_meta_u32(val, len);
>> + } else if (ops->metatype == NLA_U16) {
>> + ret = validate_meta_u16(val, len);
>> + }
>> + }
>> + }
>
>
> Sounds like this should be in a separated function.
>
Could be.
>> +int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval, int len)
>> +{
>> + struct tcf_meta_info *mi = NULL;
>> + struct tcf_meta_ops *ops = find_ife_oplist(metaid);
>> + int ret = 0;
>> +
>> + if (!ops) {
>> + return -ENOENT;
>> + }
>> +
>> + mi = kzalloc(sizeof(*mi), GFP_KERNEL);
>> + if (!mi) {
>> + /*put back what find_ife_oplist took */
>> + module_put(ops->owner);
>> + return -ENOMEM;
>> + }
>> +
>> + mi->metaid = metaid;
>> + mi->ops = ops;
>> + if (len > 0) {
>> + ret = ops->alloc(mi, metaval);
>> + if (ret != 0) {
>> + kfree(mi);
>> + module_put(ops->owner);
>> + return ret;
>> + }
>> + }
>> +
>> + /*XXX: if it becomes necessary add per tcf_meta_info lock;
>> + * for now use Thor's hammer */
>
>
> What about ife->tcf_lock?
>
I'll walk the code paths and check - it may be enough.
>
>> + write_lock(&ife_mod_lock);
>> + list_add_tail(&mi->metalist, &ife->metalist);
>> + write_unlock(&ife_mod_lock);
>> +
>> + if ((parm->flags & IFE_ENCODE) && (parm->flags & IFE_DECODE)) {
>> + pr_info("Ambigous: Cant have both encode and decode\n");
>> + return -EINVAL;
>> + }
>
>
> Is it possible to fold them into one bit?
As in the check you mean?
>> +static struct tc_action_ops act_ife_ops = {
>> + .kind = "ife",
>> + .type = TCA_ACT_IFE,
>> + .owner = THIS_MODULE,
>> + .act = tcf_ife,
>> + .dump = tcf_ife_dump,
>> + .cleanup = tcf_ife_cleanup,
>> + .init = tcf_ife_init,
>> +};
>> +
>> +static int __init ife_init_module(void)
>> +{
>> + pr_info("Loaded IFE maximum metaid %d\n", max_metacnt);
>
>
> Not needed, people can use lsmod to figure it out.
>
Yeah - missed that one after Daniel's earlier comment.
>
>> + INIT_LIST_HEAD(&ifeoplist);
>
> It is already initialized statically.
Good catch.
>> +module_param(max_metacnt, int, 0);
>
>
> I am sure DaveM doesn't like this.
>
You know what - i will take this thing out. Daniel doesnt like it
either.
Need to figure a different way to achieve the same goals.
Ok, when the noise settles i will make another release.
cheers,
jamal
next prev parent reply other threads:[~2016-02-24 13:09 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-02-23 12:49 [net-next PATCH v2 0/5] net_sched: Add support for IFE action Jamal Hadi Salim
2016-02-23 12:49 ` [net-next PATCH v2 1/5] introduce " Jamal Hadi Salim
2016-02-23 13:32 ` Daniel Borkmann
2016-02-23 14:39 ` Jamal Hadi Salim
2016-02-23 16:12 ` Daniel Borkmann
2016-02-23 21:31 ` Cong Wang
2016-02-24 5:46 ` Simon Horman
2016-02-24 12:39 ` Jamal Hadi Salim
2016-02-24 12:52 ` Jamal Hadi Salim
2016-02-23 21:44 ` Cong Wang
2016-02-24 13:09 ` Jamal Hadi Salim [this message]
2016-02-24 17:39 ` Cong Wang
2016-02-24 17:37 ` Daniel Borkmann
2016-02-25 12:20 ` Jamal Hadi Salim
2016-02-25 21:46 ` Daniel Borkmann
2016-02-25 22:07 ` John Fastabend
2016-02-25 22:46 ` Jamal Hadi Salim
2016-02-23 12:49 ` [net-next PATCH v2 2/5] Support to encoding decoding skb mark on " Jamal Hadi Salim
2016-02-23 12:49 ` [net-next PATCH v2 3/5] Support to encoding decoding skb prio " Jamal Hadi Salim
2016-02-23 12:49 ` [net-next PATCH v2 4/5] Support to encoding decoding skb hashid " Jamal Hadi Salim
2016-02-23 12:49 ` [net-next PATCH v2 5/5] Support to encoding decoding skb queue map " Jamal Hadi Salim
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=56CDAB97.1060505@mojatatu.com \
--to=jhs@mojatatu$(echo .)com \
--cc=alexei.starovoitov@gmail$(echo .)com \
--cc=daniel@iogearbox$(echo .)net \
--cc=davem@davemloft$(echo .)net \
--cc=dj@verizon$(echo .)com \
--cc=john.fastabend@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=xiyou.wangcong@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox