netdev - Re: [Patch net-next] net_sched: use idr to allocate u32 filter handles

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]

Message-ID: <CAM_iQpWbiA4QZb8j8fau7kMhzeLpRppEMcmB80byqTUryPheOw@mail.gmail.com>
Date:   Thu, 28 Sep 2017 15:19:05 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Simon Horman <simon.horman@...ronome.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Chris Mi <chrism@...lanox.com>,
        Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [Patch net-next] net_sched: use idr to allocate u32 filter handles

On Thu, Sep 28, 2017 at 12:34 AM, Simon Horman
<simon.horman@...ronome.com> wrote:
> Hi Cong,
>
> this looks like a nice enhancement to me. Did you measure any performance
> benefit from it.  Perhaps it could be described in the changelog_ I also
> have a more detailed question below.

No, I am inspired by commit c15ab236d69d, don't measure it.


>
>> ---
>>  net/sched/cls_u32.c | 108 ++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 67 insertions(+), 41 deletions(-)
>>
>> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
>> index 10b8d851fc6b..316b8a791b13 100644
>> --- a/net/sched/cls_u32.c
>> +++ b/net/sched/cls_u32.c
>> @@ -46,6 +46,7 @@
>
> ...
>
>> @@ -937,22 +940,33 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
>>                       return -EINVAL;
>>               if (TC_U32_KEY(handle))
>>                       return -EINVAL;
>> -             if (handle == 0) {
>> -                     handle = gen_new_htid(tp->data);
>> -                     if (handle == 0)
>> -                             return -ENOMEM;
>> -             }
>>               ht = kzalloc(sizeof(*ht) + divisor*sizeof(void *), GFP_KERNEL);
>>               if (ht == NULL)
>>                       return -ENOBUFS;
>> +             if (handle == 0) {
>> +                     handle = gen_new_htid(tp->data, ht);
>> +                     if (handle == 0) {
>> +                             kfree(ht);
>> +                             return -ENOMEM;
>> +                     }
>> +             } else {
>> +                     err = idr_alloc_ext(&tp_c->handle_idr, ht, NULL,
>> +                                         handle, handle + 1, GFP_KERNEL);
>> +                     if (err) {
>> +                             kfree(ht);
>> +                             return err;
>> +                     }
>
> The above seems to check that handle is not already in use and mark it as
> in use. But I don't see that logic in the code prior to this patch.
> Am I missing something? If not perhaps this portion should be a separate
> patch or described in the changelog.

The logic is in upper layer, tc_ctl_tfilter(). It tries to get a
filter by handle
(if non-zero), and errors out if we are creating a new filter with the same
handle.

At the point you quote above, 'n' is already NULL and 'handle' is non-zero,
which means there is no existing filter has same handle, it is safe to just
mark it as in-use.

Thanks.