netdev - Re: [net-next v2 3/4] openvswitch: Add meter infrastructure

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] [day] [month] [year] [list]

Date:   Wed, 18 Oct 2017 11:47:41 -0700
From:   Pravin Shelar <pshelar@....org>
To:     Andy Zhou <azhou@....org>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Joe Stringer <joe@....org>, Greg Rose <gvrose8192@...il.com>
Subject: Re: [net-next v2 3/4] openvswitch: Add meter infrastructure

On Tue, Oct 17, 2017 at 12:36 AM, Andy Zhou <azhou@....org> wrote:
> OVS kernel datapath so far does not support Openflow meter action.
> This is the first stab at adding kernel datapath meter support.
> This implementation supports only drop band type.
>
> Signed-off-by: Andy Zhou <azhou@....org>
> ---
>  net/openvswitch/Makefile   |   1 +
>  net/openvswitch/datapath.c |  14 +-
>  net/openvswitch/datapath.h |   3 +
>  net/openvswitch/meter.c    | 604 +++++++++++++++++++++++++++++++++++++++++++++
>  net/openvswitch/meter.h    |  54 ++++
>  5 files changed, 674 insertions(+), 2 deletions(-)
>  create mode 100644 net/openvswitch/meter.c
>  create mode 100644 net/openvswitch/meter.h
>
This patch mostly looks good. I have one comment below.

> +static int ovs_meter_cmd_set(struct sk_buff *skb, struct genl_info *info)
> +{
> +       struct nlattr **a = info->attrs;
> +       struct dp_meter *meter, *old_meter;
> +       struct sk_buff *reply;
> +       struct ovs_header *ovs_reply_header;
> +       struct ovs_header *ovs_header = info->userhdr;
> +       struct datapath *dp;
> +       int err;
> +       u32 meter_id;
> +       bool failed;
> +
> +       meter = dp_meter_create(a);
> +       if (IS_ERR_OR_NULL(meter))
> +               return PTR_ERR(meter);
> +
> +       reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_SET,
> +                                         &ovs_reply_header);
> +       if (IS_ERR(reply)) {
> +               err = PTR_ERR(reply);
> +               goto exit_free_meter;
> +       }
> +
> +       ovs_lock();
> +       dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> +       if (!dp) {
> +               err = -ENODEV;
> +               goto exit_unlock;
> +       }
> +
> +       if (!a[OVS_METER_ATTR_ID]) {
> +               err = -ENODEV;
> +               goto exit_unlock;
> +       }
> +
> +       meter_id = nla_get_u32(a[OVS_METER_ATTR_ID]);
> +
> +       /* Cannot fail after this. */
> +       old_meter = lookup_meter(dp, meter_id);
I do not see RCU read lock taken here. This is not correctness issue
but it could cause RCU checker to spit out warning message. You could
do same trick that is done in get_dp() to avoid this issue.

Can you also test the code with rcu sparse check config option enabled?

Thanks.