netdev - Re: [net-next RFC 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:   Mon, 16 Oct 2017 10:49:40 -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 RFC 3/4] openvswitch: Add meter infrastructure

On Mon, Oct 16, 2017 at 12:05 AM, Andy Zhou <azhou@....org> wrote:
> On Fri, Oct 13, 2017 at 5:12 PM, Pravin Shelar <pshelar@....org> wrote:
>> On Thu, Oct 12, 2017 at 3:38 PM, 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    | 611 +++++++++++++++++++++++++++++++++++++++++++++
>>>  net/openvswitch/meter.h    |  54 ++++
>>>  5 files changed, 681 insertions(+), 2 deletions(-)
>>>  create mode 100644 net/openvswitch/meter.c
>>>  create mode 100644 net/openvswitch/meter.h
>>>
>> ...
>>
>>> diff --git a/net/openvswitch/meter.c b/net/openvswitch/meter.c
>>> new file mode 100644
>>> index 000000000000..f24ebb5f7af4
>>> --- /dev/null
>>> +++ b/net/openvswitch/meter.c
>>
>> ....
>> ....
>>> +static int ovs_meter_cmd_features(struct sk_buff *skb, struct genl_info *info)
>>> +{
>>> +       struct datapath *dp;
>>> +       struct ovs_header *ovs_header = info->userhdr;
>>> +       struct sk_buff *reply;
>>> +       struct ovs_header *ovs_reply_header;
>>> +       struct nlattr *nla, *band_nla;
>>> +       int err;
>>> +
>>> +       /* Check that the datapath exists */
>>> +       ovs_lock();
>>> +       dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>>> +       ovs_unlock();
>>> +       if (!dp)
>>> +               return -ENODEV;
>>> +
>> why dp check is required for this API?
> Is it possible for another core delete the dp, before ovs_lock()
> returns? Then, in theory, get_dp() can
> return NULL, no?

I do not see dp used anywhere in function. so the question was why is
dp lookup done here?

>>
>>> +       reply = ovs_meter_cmd_reply_start(info, OVS_METER_CMD_FEATURES,
>>> +                                         &ovs_reply_header);
>>> +       if (!reply)
>>> +               return PTR_ERR(reply);
>>> +
>>> +       if (nla_put_u32(reply, OVS_METER_ATTR_MAX_METERS, U32_MAX) ||
>>> +           nla_put_u32(reply, OVS_METER_ATTR_MAX_BANDS, DP_MAX_BANDS))
>>> +               goto nla_put_failure;
>>> +
>>> +       nla = nla_nest_start(reply, OVS_METER_ATTR_BANDS);
>>> +       if (!nla)
>>> +               goto nla_put_failure;
>>> +
>>> +       band_nla = nla_nest_start(reply, OVS_BAND_ATTR_UNSPEC);
>>> +       if (!band_nla)
>>> +               goto nla_put_failure;
>>> +       /* Currently only DROP band type is supported. */
>>> +       if (nla_put_u32(reply, OVS_BAND_ATTR_TYPE, OVS_METER_BAND_TYPE_DROP))
>>> +               goto nla_put_failure;
>>> +       nla_nest_end(reply, band_nla);
>>> +       nla_nest_end(reply, nla);
>>> +
>>> +       genlmsg_end(reply, ovs_reply_header);
>>> +       return genlmsg_reply(reply, info);
>>> +
>>> +nla_put_failure:
>>> +       nlmsg_free(reply);
>>> +       err = -EMSGSIZE;
>>> +       return err;
>>> +}
>>> +
>> ....
>>
>>> +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);
>>> +       attach_meter(dp, meter);
>>> +       ovs_unlock();
>>> +
>> After the unlock, it is not safe to keep the ref to old_meter. better
>> to release lock at the end. we could optimize it later if required.
>
> I see a problem here: the old_meter has not been removed from the list before
> unlock. The code should have been:
>
> old_meter = lookup_meter(dp, meter_id);
> detch_meter(dp, old_meter);
> attach_meter(dp, meter);
> ovs_unlock();
>
> Do you still see a problem w.r.t. unlock() here? Once detch_meter() is called,
> another thread should not have access to 'old_meter' any more. right?

looks good.