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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAOrHB_AuQ5q8Myp_A4hDOtnKmjgzW7bf9dnfdqX91n6iizV+BQ@mail.gmail.com>
Date:   Thu, 2 Nov 2017 05:07:05 -0700
From:   Pravin Shelar <pshelar@....org>
To:     Andy Zhou <azhou@....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 Thu, Nov 2, 2017 at 3:07 AM, Andy Zhou <azhou@....org> wrote:
> 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?

There would be assert failure and stack trace. so it would be pretty
obvious in kernel log messages.
Let me know if you do not see any stack trace while running meter
create, delete and execute.