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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 26 Feb 2016 15:49:05 -0800
From:	Cong Wang <xiyou.wangcong@...il.com>
To:	Jamal Hadi Salim <jhs@...atatu.com>
Cc:	David Miller <davem@...emloft.net>,
	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Daniel Borkmann <daniel@...earbox.net>
Subject: Re: [net-next-2.6 v3 1/3] introduce IFE action

On Fri, Feb 26, 2016 at 2:43 PM, Jamal Hadi Salim <jhs@...atatu.com> wrote:

[...]


Just some quick reviews... ;)


> +#define IFE_TAB_MASK 15
> +
> +static int ife_net_id;
> +static int max_metacnt = IFE_META_MAX + 1;
> +
> +static const struct nla_policy ife_policy[TCA_IFE_MAX + 1] = {
> +       [TCA_IFE_PARMS] = { .len = sizeof(struct tc_ife)},
> +       [TCA_IFE_DMAC] = { .len = ETH_ALEN},
> +       [TCA_IFE_SMAC] = { .len = ETH_ALEN},
> +       [TCA_IFE_TYPE] = { .type = NLA_U16},
> +};
> +
> +/* Caller takes care of presenting data in network order
> +*/
> +int ife_tlv_meta_encode(void *skbdata, u16 attrtype, u16 dlen, const void *dval)
> +{
> +       u32 *tlv = (u32 *)(skbdata);
> +       u16 totlen = nla_total_size(dlen);      /*alignment + hdr */
> +       char *dptr = (char *)tlv + NLA_HDRLEN;
> +       u32 htlv = attrtype << 16 | totlen;
> +
> +       *tlv = htonl(htlv);
> +       memset(dptr, 0, totlen - NLA_HDRLEN);
> +       memcpy(dptr, dval, dlen);
> +
> +       return totlen;
> +}
> +EXPORT_SYMBOL_GPL(ife_tlv_meta_encode);
> +
> +int ife_get_meta_u32(struct sk_buff *skb, struct tcf_meta_info *mi)
> +{
> +       if (mi->metaval)
> +               return nla_put_u32(skb, mi->metaid, *(u32 *)mi->metaval);
> +       else
> +               return nla_put(skb, mi->metaid, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(ife_get_meta_u32);
> +
> +int ife_check_meta_u32(u32 metaval, struct tcf_meta_info *mi)
> +{
> +       if (metaval || mi->metaval)
> +               return 8; /* T+L+V == 2+2+4 */
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(ife_check_meta_u32);
> +
> +int ife_encode_meta_u32(u32 metaval, void *skbdata, struct tcf_meta_info *mi)
> +{
> +       u32 edata = metaval;
> +
> +       if (mi->metaval)
> +               edata = *(u32 *)mi->metaval;
> +       else if (metaval)
> +               edata = metaval;
> +
> +       if (!edata) /* will not encode */
> +               return 0;
> +
> +       edata = htonl(edata);
> +       return ife_tlv_meta_encode(skbdata, mi->metaid, 4, &edata);
> +}
> +EXPORT_SYMBOL_GPL(ife_encode_meta_u32);
> +
> +int ife_get_meta_u16(struct sk_buff *skb, struct tcf_meta_info *mi)
> +{
> +       if (mi->metaval)
> +               return nla_put_u16(skb, mi->metaid, *(u16 *)mi->metaval);
> +       else
> +               return nla_put(skb, mi->metaid, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(ife_get_meta_u16);
> +
> +int ife_alloc_meta_u32(struct tcf_meta_info *mi, void *metaval)
> +{
> +       mi->metaval = kmemdup(&metaval, sizeof(u32), GFP_KERNEL);
> +       if (!mi->metaval)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(ife_alloc_meta_u32);
> +
> +int ife_alloc_meta_u16(struct tcf_meta_info *mi, void *metaval)
> +{
> +       mi->metaval = kmemdup(&metaval, sizeof(u16), GFP_KERNEL);
> +       if (!mi->metaval)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(ife_alloc_meta_u16);
> +
> +void ife_release_meta_gen(struct tcf_meta_info *mi)
> +{
> +       kfree(mi->metaval);
> +}
> +EXPORT_SYMBOL_GPL(ife_release_meta_gen);
> +
> +int ife_validate_meta_u32(void *val, int len)
> +{
> +       if (len == 4)
> +               return 0;
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(ife_validate_meta_u32);
> +
> +int ife_validate_meta_u16(void *val, int len)
> +{
> +       /* length will include padding */
> +       if (len == NLA_ALIGN(2))
> +               return 0;
> +
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(ife_validate_meta_u16);
> +
> +static LIST_HEAD(ifeoplist);
> +static DEFINE_RWLOCK(ife_mod_lock);
> +
> +struct tcf_meta_ops *find_ife_oplist(u16 metaid)


static?


> +{
> +       struct tcf_meta_ops *o;
> +
> +       read_lock(&ife_mod_lock);
> +       list_for_each_entry(o, &ifeoplist, list) {
> +               if (o->metaid == metaid) {
> +                       if (!try_module_get(o->owner))
> +                               o = NULL;
> +                       read_unlock(&ife_mod_lock);
> +                       return o;
> +               }
> +       }
> +       read_unlock(&ife_mod_lock);
> +
> +       return NULL;
> +}
> +
> +int register_ife_op(struct tcf_meta_ops *mops)
> +{
> +       struct tcf_meta_ops *m;
> +
> +       if (!mops->metaid || !mops->metatype || !mops->name ||
> +           !mops->check_presence || !mops->encode || !mops->decode ||
> +           !mops->get || !mops->alloc)
> +               return -EINVAL;
> +
> +       write_lock(&ife_mod_lock);
> +
> +       list_for_each_entry(m, &ifeoplist, list) {
> +               if (m->metaid == mops->metaid ||
> +                   (strcmp(mops->name, m->name) == 0)) {
> +                       write_unlock(&ife_mod_lock);
> +                       return -EEXIST;
> +               }
> +       }
> +
> +       if (!mops->release)
> +               mops->release = ife_release_meta_gen;
> +
> +       list_add_tail(&mops->list, &ifeoplist);
> +       write_unlock(&ife_mod_lock);
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(unregister_ife_op);
> +
> +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);
> +
> +void print_ife_oplist(void)

static?

> +{
> +       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);
> +       }


Is this really useful? Modules can be found by lsmod.


> +       read_unlock(&ife_mod_lock);
> +}
> +
> +static int ife_validate_metatype(struct tcf_meta_ops *ops, void *val, int len)
> +{
> +       int 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 (ops->validate)
> +               return ops->validate(val, len);
> +
> +       if (ops->metatype == NLA_U32)
> +               ret = ife_validate_meta_u32(val, len);
> +       else if (ops->metatype == NLA_U16)
> +               ret = ife_validate_meta_u16(val, len);
> +
> +       return ret;
> +}
> +
> +/* 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)


Can become static?


> +{
> +       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;
> +               if (len)
> +                       ret = ife_validate_metatype(ops, val, len);
> +
> +               module_put(ops->owner);
> +       }
> +
> +       return ret;
> +}
> +
> +/* called when adding new meta information
> + * under ife->tcf_lock
> +*/
> +int add_metainfo(struct tcf_ife_info *ife, u32 metaid, void *metaval, int len)
> +{


static?


> +       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;
> +               }
> +       }
> +
> +       list_add_tail(&mi->metalist, &ife->metalist);
> +
> +       return ret;
> +}
> +
> +int use_all_metadata(struct tcf_ife_info *ife)


static?

> +{
> +       struct tcf_meta_ops *o;
> +       int rc = 0;
> +       int installed = 0;
> +
> +       list_for_each_entry(o, &ifeoplist, list) {
> +               rc = add_metainfo(ife, o->metaid, NULL, 0);
> +               if (rc == 0)
> +                       installed += 1;
> +       }
> +
> +       if (installed)
> +               return 0;
> +       else
> +               return -EINVAL;
> +}
> +
> +int dump_metalist(struct sk_buff *skb, struct tcf_ife_info *ife)


static?


> +{
> +       struct tcf_meta_info *e;
> +       struct nlattr *nest;
> +       unsigned char *b = skb_tail_pointer(skb);
> +       int total_encoded = 0;
> +
> +       /*can only happen on decode */
> +       if (list_empty(&ife->metalist))
> +               return 0;
> +
> +       nest = nla_nest_start(skb, TCA_IFE_METALST);
> +       if (!nest)
> +               goto out_nlmsg_trim;
> +
> +       list_for_each_entry(e, &ife->metalist, metalist) {
> +               if (!e->ops->get(skb, e))
> +                       total_encoded += 1;
> +       }
> +
> +       if (!total_encoded)
> +               goto out_nlmsg_trim;
> +
> +       nla_nest_end(skb, nest);
> +
> +       return 0;
> +
> +out_nlmsg_trim:
> +       nlmsg_trim(skb, b);
> +       return -1;
> +}
> +
> +/* under ife->tcf_lock */
> +static void _tcf_ife_cleanup(struct tc_action *a, int bind)
> +{
> +       struct tcf_ife_info *ife = a->priv;
> +       struct tcf_meta_info *e, *n;
> +
> +       list_for_each_entry_safe(e, n, &ife->metalist, metalist) {
> +               module_put(e->ops->owner);
> +               list_del(&e->metalist);
> +               if (e->metaval) {
> +                       if (e->ops->release)
> +                               e->ops->release(e);
> +                       else
> +                               kfree(e->metaval);
> +               }
> +               kfree(e);
> +       }
> +}
> +
> +static void tcf_ife_cleanup(struct tc_action *a, int bind)
> +{
> +       struct tcf_ife_info *ife = a->priv;
> +
> +       spin_lock_bh(&ife->tcf_lock);
> +       _tcf_ife_cleanup(a, bind);
> +       spin_unlock_bh(&ife->tcf_lock);
> +}
> +
> +/* under ife->tcf_lock */
> +int populate_metalist(struct tcf_ife_info *ife, struct nlattr **tb)


static?


> +{
> +       int len = 0;
> +       int rc = 0;
> +       int i = 0;
> +       void *val;
> +
> +       for (i = 1; i < max_metacnt; i++) {
> +               if (tb[i]) {
> +                       val = nla_data(tb[i]);
> +                       len = nla_len(tb[i]);
> +
> +                       rc = load_metaops_and_vet(ife, i, val, len);
> +                       if (rc != 0)
> +                               return rc;
> +
> +                       rc = add_metainfo(ife, i, val, len);
> +                       if (rc)
> +                               return rc;
> +               }
> +       }
> +
> +       return rc;
> +}
> +
> +static int tcf_ife_init(struct net *net, struct nlattr *nla,
> +                       struct nlattr *est, struct tc_action *a,
> +                       int ovr, int bind)
> +{
> +       struct tc_action_net *tn = net_generic(net, ife_net_id);
> +       struct nlattr *tb[TCA_IFE_MAX + 1];
> +       struct nlattr *tb2[IFE_META_MAX + 1];
> +       struct tcf_ife_info *ife;
> +       struct tc_ife *parm;
> +       u16 ife_type = 0;
> +       u8 *daddr = NULL;
> +       u8 *saddr = NULL;
> +       int ret = 0;
> +       int err;
> +
> +       err = nla_parse_nested(tb, TCA_IFE_MAX, nla, ife_policy);
> +       if (err < 0)
> +               return err;
> +
> +       if (!tb[TCA_IFE_PARMS])
> +               return -EINVAL;
> +
> +       parm = nla_data(tb[TCA_IFE_PARMS]);
> +
> +       if (parm->flags & IFE_ENCODE) {
> +               /* Until we get issued the ethertype, we cant have
> +                * a default..
> +               **/
> +               if (!tb[TCA_IFE_TYPE]) {
> +                       pr_info("You MUST pass etherype for encoding\n");
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       if (!tcf_hash_check(tn, parm->index, a, bind)) {
> +               ret = tcf_hash_create(tn, parm->index, est, a, sizeof(*ife),
> +                                     bind, false);
> +               if (ret)
> +                       return ret;
> +               ret = ACT_P_CREATED;
> +       } else {
> +               if (bind)       /* dont override defaults */
> +                       return 0;
> +               tcf_hash_release(a, bind);
> +               if (!ovr)
> +                       return -EEXIST;
> +       }
> +
> +       ife = to_ife(a);
> +       ife->flags = parm->flags;
> +
> +       if (parm->flags & IFE_ENCODE) {
> +               ife_type = nla_get_u16(tb[TCA_IFE_TYPE]);
> +               if (tb[TCA_IFE_DMAC])
> +                       daddr = nla_data(tb[TCA_IFE_DMAC]);
> +               if (tb[TCA_IFE_SMAC])
> +                       saddr = nla_data(tb[TCA_IFE_SMAC]);
> +       }
> +
> +       spin_lock_bh(&ife->tcf_lock);
> +       ife->tcf_action = parm->action;
> +
> +       if (parm->flags & IFE_ENCODE) {
> +               if (daddr)
> +                       ether_addr_copy(ife->eth_dst, daddr);
> +               else
> +                       eth_zero_addr(ife->eth_dst);
> +
> +               if (saddr)
> +                       ether_addr_copy(ife->eth_src, saddr);
> +               else
> +                       eth_zero_addr(ife->eth_src);
> +
> +               ife->eth_type = ife_type;
> +       }
> +
> +       if (ret == ACT_P_CREATED)
> +               INIT_LIST_HEAD(&ife->metalist);
> +
> +       if (tb[TCA_IFE_METALST]) {
> +               err = nla_parse_nested(tb2, IFE_META_MAX, tb[TCA_IFE_METALST],
> +                                      NULL);
> +               if (err) {
> +metadata_parse_err:
> +                       if (ret == ACT_P_CREATED)
> +                               _tcf_ife_cleanup(a, bind);
> +
> +                       spin_unlock_bh(&ife->tcf_lock);
> +                       return err;
> +               }
> +
> +               err = populate_metalist(ife, tb2);
> +               if (err)
> +                       goto metadata_parse_err;
> +
> +       } else {
> +               /* if no passed metadata allow list or passed allow-all
> +                * then here we process by adding as many supported metadatum
> +                * as we can. You better have at least one else we are
> +                * going to bail out
> +                */
> +               err = use_all_metadata(ife);
> +               if (err) {
> +                       if (ret == ACT_P_CREATED)
> +                               _tcf_ife_cleanup(a, bind);
> +
> +                       spin_unlock_bh(&ife->tcf_lock);
> +                       return err;
> +               }
> +       }
> +
> +       spin_unlock_bh(&ife->tcf_lock);
> +
> +       if (ret == ACT_P_CREATED)
> +               tcf_hash_insert(tn, a);
> +
> +       print_ife_oplist();
> +       return ret;
> +}
> +
> +static int tcf_ife_dump(struct sk_buff *skb, struct tc_action *a, int bind,
> +                       int ref)
> +{
> +       unsigned char *b = skb_tail_pointer(skb);
> +       struct tcf_ife_info *ife = a->priv;
> +       struct tc_ife opt = {
> +               .index = ife->tcf_index,
> +               .refcnt = ife->tcf_refcnt - ref,
> +               .bindcnt = ife->tcf_bindcnt - bind,
> +               .action = ife->tcf_action,
> +               .flags = ife->flags,
> +       };
> +       struct tcf_t t;
> +
> +       if (nla_put(skb, TCA_IFE_PARMS, sizeof(opt), &opt))
> +               goto nla_put_failure;
> +
> +       t.install = jiffies_to_clock_t(jiffies - ife->tcf_tm.install);
> +       t.lastuse = jiffies_to_clock_t(jiffies - ife->tcf_tm.lastuse);
> +       t.expires = jiffies_to_clock_t(ife->tcf_tm.expires);
> +       if (nla_put(skb, TCA_IFE_TM, sizeof(t), &t))
> +               goto nla_put_failure;
> +
> +       if (!is_zero_ether_addr(ife->eth_dst)) {
> +               if (nla_put(skb, TCA_IFE_DMAC, ETH_ALEN, ife->eth_dst))
> +                       goto nla_put_failure;
> +       }
> +
> +       if (!is_zero_ether_addr(ife->eth_src)) {
> +               if (nla_put(skb, TCA_IFE_SMAC, ETH_ALEN, ife->eth_src))
> +                       goto nla_put_failure;
> +       }
> +
> +       if (nla_put(skb, TCA_IFE_TYPE, 2, &ife->eth_type))
> +               goto nla_put_failure;
> +
> +       if (dump_metalist(skb, ife)) {
> +               /*ignore failure to dump metalist */
> +               pr_info("Failed to dump metalist\n");
> +       }


Make it void?


> +
> +       return skb->len;
> +
> +nla_put_failure:
> +       nlmsg_trim(skb, b);
> +       return -1;
> +}
> +
> +int find_decode_metaid(struct sk_buff *skb, struct tcf_ife_info *ife,
> +                      u16 metaid, u16 mlen, void *mdata)
> +{
> +       struct tcf_meta_info *e;
> +
> +       /* XXX: use hash to speed up */
> +       list_for_each_entry(e, &ife->metalist, metalist) {
> +               if (metaid == e->metaid) {
> +                       if (e->ops) {
> +                               /* We check for decode presence already */
> +                               return e->ops->decode(skb, mdata, mlen);
> +                       }
> +               }
> +       }
> +
> +       return 0;
> +}
> +
> +struct ifeheadr {
> +       __be16 metalen;
> +       u8 tlv_data[];
> +};
> +
> +struct meta_tlvhdr {
> +       __be16 type;
> +       __be16 len;
> +};
> +
> +static int tcf_ife_decode(struct sk_buff *skb, const struct tc_action *a,
> +                         struct tcf_result *res)
> +{
> +       struct tcf_ife_info *ife = a->priv;
> +       int action = ife->tcf_action;
> +       struct ifeheadr *ifehdr = (struct ifeheadr *)skb->data;
> +       u16 ifehdrln = ifehdr->metalen;
> +       struct meta_tlvhdr *tlv = (struct meta_tlvhdr *)(ifehdr->tlv_data);
> +
> +       spin_lock(&ife->tcf_lock);
> +       bstats_update(&ife->tcf_bstats, skb);
> +       ife->tcf_tm.lastuse = jiffies;
> +       spin_unlock(&ife->tcf_lock);
> +
> +       ifehdrln = ntohs(ifehdrln);
> +       if (unlikely(!pskb_may_pull(skb, ifehdrln))) {
> +               spin_lock(&ife->tcf_lock);
> +               ife->tcf_qstats.drops++;
> +               spin_unlock(&ife->tcf_lock);
> +               return TC_ACT_SHOT;
> +       }
> +
> +       skb_set_mac_header(skb, ifehdrln);
> +       __skb_pull(skb, ifehdrln);
> +       skb->protocol = eth_type_trans(skb, skb->dev);
> +       ifehdrln -= IFE_METAHDRLEN;
> +
> +       while (ifehdrln > 0) {
> +               u8 *tlvdata = (u8 *)tlv;
> +               u16 mtype = tlv->type;
> +               u16 mlen = tlv->len;
> +
> +               mtype = ntohs(mtype);
> +               mlen = ntohs(mlen);
> +
> +               if (find_decode_metaid(skb, ife, mtype, (mlen - 4),
> +                                      (void *)(tlvdata + 4))) {
> +                       /* abuse overlimits to count when we receive metadata
> +                        * but dont have an ops for it
> +                        */
> +                       pr_info_ratelimited("Unknown metaid %d alnlen %d\n",
> +                                           mtype, mlen);
> +                       ife->tcf_qstats.overlimits++;
> +               }
> +
> +               tlvdata += mlen;
> +               ifehdrln -= mlen;
> +               tlv = (struct meta_tlvhdr *)tlvdata;
> +       }
> +
> +       skb_reset_network_header(skb);
> +       return action;
> +}
> +
> +/*XXX: check if we can do this at install time instead of current
> + * send data path
> +**/
> +static int ife_get_sz(struct sk_buff *skb, struct tcf_ife_info *ife)
> +{
> +       struct tcf_meta_info *e, *n;
> +       int tot_run_sz = 0, run_sz = 0;
> +
> +       list_for_each_entry_safe(e, n, &ife->metalist, metalist) {
> +               if (e->ops->check_presence) {
> +                       run_sz = e->ops->check_presence(skb, e);
> +                       tot_run_sz += run_sz;
> +               }
> +       }
> +
> +       return tot_run_sz;
> +}
> +
> +static int tcf_ife_encode(struct sk_buff *skb, const struct tc_action *a,
> +                         struct tcf_result *res)
> +{
> +       struct tcf_ife_info *ife = a->priv;
> +       int action = ife->tcf_action;
> +       struct ethhdr *oethh;   /* outer ether header */
> +       struct ethhdr *iethh;   /* inner eth header */
> +       struct tcf_meta_info *e;
> +       /*
> +          OUTERHDR:TOTMETALEN:{TLVHDR:Metadatum:TLVHDR..}:ORIGDATA
> +          where ORIGDATA = original ethernet header ...
> +        */
> +       u16 metalen = ife_get_sz(skb, ife);
> +       int hdrm = metalen + skb->dev->hard_header_len + IFE_METAHDRLEN;
> +       unsigned int skboff = skb->dev->hard_header_len;
> +       u32 at = G_TC_AT(skb->tc_verd);
> +       int new_len = skb->len + hdrm;
> +       int exceed_mtu = 0;

Boolean?


> +       int err;
> +
> +       if (at & AT_EGRESS) {
> +               if (new_len > skb->dev->mtu)
> +                       exceed_mtu = 1;
> +       }
> +
> +       spin_lock(&ife->tcf_lock);
> +       bstats_update(&ife->tcf_bstats, skb);
> +       ife->tcf_tm.lastuse = jiffies;
> +
> +       if (!metalen) {         /* no metadata to send */
> +               spin_unlock(&ife->tcf_lock);
> +               /* abuse overlimits to count when we allow packet
> +                * with no metadata
> +                */
> +               ife->tcf_qstats.overlimits++;


Update before unlocking?

> +               return action;
> +       }
> +       /* could be stupid policy setup or mtu config
> +        * so lets be conservative.. */
> +       if ((action == TC_ACT_SHOT) || exceed_mtu) {
> +               ife->tcf_qstats.drops++;
> +               spin_unlock(&ife->tcf_lock);
> +               return TC_ACT_SHOT;
> +       }
> +
> +       iethh = eth_hdr(skb);
> +
> +       err = skb_cow_head(skb, hdrm);
> +       if (unlikely(err)) {
> +               ife->tcf_qstats.drops++;
> +               spin_unlock(&ife->tcf_lock);
> +               return TC_ACT_SHOT;
> +       }
> +
> +       if (!(at & AT_EGRESS)) {
> +               skb_push(skb, skb->dev->hard_header_len);
> +       }


braces {} are not necessary for single statement blocks


> +
> +       __skb_push(skb, hdrm);
> +       memcpy(skb->data, iethh, skb->mac_len);
> +       skb_reset_mac_header(skb);
> +       oethh = eth_hdr(skb);
> +
> +       /*total metadata length */
> +       metalen += IFE_METAHDRLEN;
> +       metalen = htons(metalen);
> +       memcpy((void *)(skb->data + skboff), &metalen, IFE_METAHDRLEN);

Unnecessary cast to void* ?


> +       skboff += IFE_METAHDRLEN;
> +
> +       /* XXX: we dont have a clever way of telling encode to
> +        * not repeat some of the computations that are done by
> +        * ops->presence_check...
> +        */
> +       list_for_each_entry(e, &ife->metalist, metalist) {
> +               if (e->ops->encode) {
> +                       err = e->ops->encode(skb, (void *)(skb->data + skboff),
> +                                            e);
> +               }
> +               if (err < 0) {
> +                       /* too corrupt to keep around if overwritten */
> +                       ife->tcf_qstats.drops++;
> +                       spin_unlock(&ife->tcf_lock);
> +                       return TC_ACT_SHOT;
> +               }
> +               skboff += err;
> +       }
> +
> +       if (!is_zero_ether_addr(ife->eth_src))
> +               ether_addr_copy(oethh->h_source, ife->eth_src);
> +       else
> +               ether_addr_copy(oethh->h_source, iethh->h_source);
> +       if (!is_zero_ether_addr(ife->eth_dst))
> +               ether_addr_copy(oethh->h_dest, ife->eth_dst);
> +       else
> +               ether_addr_copy(oethh->h_dest, iethh->h_dest);
> +       oethh->h_proto = htons(ife->eth_type);
> +
> +       if (!(at & AT_EGRESS))
> +               skb_pull(skb, skb->dev->hard_header_len);
> +
> +       spin_unlock(&ife->tcf_lock);
> +
> +       return action;
> +}
> +
> +static int tcf_ife_act(struct sk_buff *skb, const struct tc_action *a,
> +                      struct tcf_result *res)
> +{
> +       struct tcf_ife_info *ife = a->priv;
> +
> +       if (ife->flags & IFE_ENCODE)
> +               return tcf_ife_encode(skb, a, res);
> +
> +       if (!(ife->flags & IFE_ENCODE))
> +               return tcf_ife_decode(skb, a, res);
> +
> +       pr_info("unknown failure(policy neither de/encode\n");


ratelimit?


> +       spin_lock(&ife->tcf_lock);
> +       bstats_update(&ife->tcf_bstats, skb);
> +       ife->tcf_tm.lastuse = jiffies;
> +       ife->tcf_qstats.drops++;
> +       spin_unlock(&ife->tcf_lock);
> +
> +       return TC_ACT_SHOT;
> +}
> +
> +static int tcf_ife_walker(struct net *net, struct sk_buff *skb,
> +                         struct netlink_callback *cb, int type,
> +                         struct tc_action *a)
> +{
> +       struct tc_action_net *tn = net_generic(net, ife_net_id);
> +
> +       return tcf_generic_walker(tn, skb, cb, type, a);
> +}
> +
> +static int tcf_ife_search(struct net *net, struct tc_action *a, u32 index)
> +{
> +       struct tc_action_net *tn = net_generic(net, ife_net_id);
> +
> +       return tcf_hash_search(tn, a, index);
> +}
> +
> +static struct tc_action_ops act_ife_ops = {
> +       .kind = "ife",
> +       .type = TCA_ACT_IFE,
> +       .owner = THIS_MODULE,
> +       .act = tcf_ife_act,
> +       .dump = tcf_ife_dump,
> +       .cleanup = tcf_ife_cleanup,
> +       .init = tcf_ife_init,
> +       .walk = tcf_ife_walker,
> +       .lookup = tcf_ife_search,
> +};
> +
> +static __net_init int ife_init_net(struct net *net)
> +{
> +       struct tc_action_net *tn = net_generic(net, ife_net_id);
> +
> +       return tc_action_net_init(tn, &act_ife_ops, IFE_TAB_MASK);
> +}
> +
> +static void __net_exit ife_exit_net(struct net *net)
> +{
> +       struct tc_action_net *tn = net_generic(net, ife_net_id);
> +
> +       tc_action_net_exit(tn);
> +}
> +
> +static struct pernet_operations ife_net_ops = {
> +       .init = ife_init_net,
> +       .exit = ife_exit_net,
> +       .id   = &ife_net_id,
> +       .size = sizeof(struct tc_action_net),
> +};
> +
> +MODULE_AUTHOR("Jamal Hadi Salim(2015)");
> +MODULE_DESCRIPTION("Inter-FE LFB action");
> +MODULE_LICENSE("GPL");


Move these to the bottom?


> +
> +static int __init ife_init_module(void)
> +{
> +       return tcf_register_action(&act_ife_ops, &ife_net_ops);
> +}
> +
> +static void __exit ife_cleanup_module(void)
> +{
> +       tcf_unregister_action(&act_ife_ops, &ife_net_ops);
> +}
> +
> +module_init(ife_init_module);
> +module_exit(ife_cleanup_module);


Thanks!

Powered by blists - more mailing lists