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

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

On Fri, Oct 20, 2017 at 8:32 PM, Pravin Shelar <pshelar@....org> wrote:
> On Thu, Oct 19, 2017 at 5:58 PM, Andy Zhou <azhou@....org> wrote:
>>
>> On Thu, Oct 19, 2017 at 02:47 Pravin Shelar <pshelar@....org> wrote:
>>>
>>> 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.
>>
>> O.K.
>>>
>>>
>>>
>>> Can you also test the code with rcu sparse check config option enabled?
>>
>>
>> Do you mean to sparse compile with CONFIG_PROVE_LOCKING and
>> CONFIG_DENUG_OBJECTS_RCU_HEAD?
>
> You could use all following options simultaneously:
> CONFIG_PREEMPT
> CONFIG_DEBUG_PREEMPT
> CONFIG_DEBUG_SPINLOCK
> CONFIG_DEBUG_ATOMIC_SLEEP
> CONFIG_PROVE_RCU
> CONFIG_DEBUG_OBJECTS_RCU_HEAD

Thanks, I turned on those flags but did not get any error message. Do you
mind share the RCU checker message?