netdev - Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of xt

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: <CANP3RGeo71YYqPENVkEC_XzS+44ANQ9gLV2eX=aZbuc6TMuTAQ@mail.gmail.com>
Date:   Tue, 2 Oct 2018 01:24:29 -0700
From:   Maciej Żenczykowski <zenczykowski@...il.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     Chenbo Feng <chenbofeng.kernel@...il.com>,
        Linux NetDev <netdev@...r.kernel.org>,
        netfilter-devel@...r.kernel.org, kernel-team@...roid.com,
        Lorenzo Colitti <lorenzo@...gle.com>,
        Chenbo Feng <fengc@...gle.com>,
        Maciej Żenczykowski <zenczykowski@...il.com>,
        Maciej Zenczykowski <maze@...gle.com>
Subject: Re: [PATCH net-next] netfilter: xt_quota: fix the behavior of
 xt_quota module

> A few questions, see below.
>
> First one is, don't we need a new match revision for this new option?

We were very careful to do this in a way that doesn't need a new revision.

That's what basically dictates most of the design.

If we bump the revision then you don't get fixed semantics unless
you update both kernel and userspace iptables versions... and additionally
we basically end up with two copies of xt_quota in the kernel source since
there's pretty much nothing that can be shared between the two.

> So 1 means, don't keep updating, quota is depleted?

The counter is always 1 higher then the remaining quota.
So 1 means 0.
And 0 means uninitialized and is only present on input from userspace.

> This current_count = 1 would be exposed to userspace too, right?
>
> Hm, this semantics are going to be a bit awkwards to users I think, I
> would prefer to expose this in a different way.

New userspace iptables hides this from users by adding/decrementing by
one as needed on ingress/egress from kernel.
Old iptables never looks at this field.

>> +             if (current_count <= skb->len) {
>> +                     atomic64_set(&q->counter, 1);
>> +                     return ret;
>> +             }
>> +             old_count = current_count;
>> +             new_count = current_count - skb->len;
>> +             current_count = atomic64_cmpxchg(&q->counter, old_count,
>> +                                              new_count);
>> +     } while (current_count != old_count);
>
> Probably we simplify this via atomic64_add_return()?

Unfortunately that doesn't work because it's racy if current value is
2 and two (or three) threads both add -1 you end up at zero (or even
+lots).

> I guess problem is userspace may get a current counter that is larger
> than the quota, but we could handle this from userspace iptables to
> print a value that equals the quota, ie. from userspace, before
> printing:

I'm not sure what you mean.

>
>         if (consumed > quota)
>                 printf("--consumed %PRIu64 ", quota);
>         else
>                 printf("--consumed %PRIu64 ", consumed);
>
>> +     return !ret;
>>  }
>
> Thanks !

Maciej Żenczykowski, Kernel Networking Developer @ Google