Re: [PATCH 1/2] netfilter: nfnetlink_acct: Adding quota support to accounting framework

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 7 April 2014 09:20, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> Hi Mathieu,
>
> On Tue, Apr 01, 2014 at 04:52:51PM -0600, mathieu.poirier@xxxxxxxxxx wrote:
>> From: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>>
>> Nf_acct objects already support accounting at the byte and packet
>> level.  As such it is a natural extention to add the possiblity to
>> define a ceiling limit for both metrics.
>>
>> All the support for quotas itself is added to nfnetlink acctounting
>> framework to stay coherent with current accounting object management.
>> Quota limit checks are implemented in xt nfacct filter where
>> statistic collection is already done.
>
> This looks good, we didn't reach the merge window though (it closed
> the same day you sent me this). So please address some minor issues
> here and resend, I'll keep this in my local tree until the net-next
> merge window opens back.

Perfect.

>
> As said, comments below.
>
>> Pablo Niera Ayuso has also contributed to this feature.
>>
>> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>> ---
>>  include/linux/netfilter/nfnetlink_acct.h      |  8 ++-
>>  include/uapi/linux/netfilter/nfnetlink.h      |  2 +
>>  include/uapi/linux/netfilter/nfnetlink_acct.h |  9 +++
>>  net/netfilter/nfnetlink_acct.c                | 86 +++++++++++++++++++++++++++
>>  net/netfilter/xt_nfacct.c                     |  6 ++
>>  5 files changed, 110 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
>> index b2e85e5..6ec9757 100644
>> --- a/include/linux/netfilter/nfnetlink_acct.h
>> +++ b/include/linux/netfilter/nfnetlink_acct.h
>> @@ -3,11 +3,17 @@
>>
>>  #include <uapi/linux/netfilter/nfnetlink_acct.h>
>>
>> +enum {
>> +     NFACCT_NO_QUOTA         = -1,
>> +     NFACCT_UNDERQUOTA,
>> +     NFACCT_OVERQUOTA,
>> +};
>>
>>  struct nf_acct;
>>
>>  struct nf_acct *nfnl_acct_find_get(const char *filter_name);
>>  void nfnl_acct_put(struct nf_acct *acct);
>>  void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct);
>> -
>> +extern int nfnl_acct_overquota(const struct sk_buff *skb,
>> +                           struct nf_acct *nfacct);
>>  #endif /* _NFNL_ACCT_H */
>> diff --git a/include/uapi/linux/netfilter/nfnetlink.h b/include/uapi/linux/netfilter/nfnetlink.h
>> index 596ddd4..354a7e5 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink.h
>> @@ -20,6 +20,8 @@ enum nfnetlink_groups {
>>  #define NFNLGRP_CONNTRACK_EXP_DESTROY        NFNLGRP_CONNTRACK_EXP_DESTROY
>>       NFNLGRP_NFTABLES,
>>  #define NFNLGRP_NFTABLES                NFNLGRP_NFTABLES
>> +     NFNLGRP_ACCT_QUOTA,
>> +#define NFNLGRP_ACCT_QUOTA           NFNLGRP_ACCT_QUOTA
>>       __NFNLGRP_MAX,
>>  };
>>  #define NFNLGRP_MAX  (__NFNLGRP_MAX - 1)
>> diff --git a/include/uapi/linux/netfilter/nfnetlink_acct.h b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> index c7b6269..51404ec 100644
>> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
>> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
>> @@ -10,15 +10,24 @@ enum nfnl_acct_msg_types {
>>       NFNL_MSG_ACCT_GET,
>>       NFNL_MSG_ACCT_GET_CTRZERO,
>>       NFNL_MSG_ACCT_DEL,
>> +     NFNL_MSG_ACCT_OVERQUOTA,
>>       NFNL_MSG_ACCT_MAX
>>  };
>>
>> +enum nfnl_acct_flags {
>> +     NFACCT_F_QUOTA_PKTS     = (1 << 0),
>> +     NFACCT_F_QUOTA_BYTES    = (1 << 1),
>> +     NFACCT_F_OVERQUOTA      = (1 << 2), /* can't be set from userspace */
>> +};
>> +
>>  enum nfnl_acct_type {
>>       NFACCT_UNSPEC,
>>       NFACCT_NAME,
>>       NFACCT_PKTS,
>>       NFACCT_BYTES,
>>       NFACCT_USE,
>> +     NFACCT_FLAGS,
>> +     NFACCT_QUOTA,
>>       __NFACCT_MAX
>>  };
>>  #define NFACCT_MAX (__NFACCT_MAX - 1)
>> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
>> index c7b6d46..dae35eb 100644
>> --- a/net/netfilter/nfnetlink_acct.c
>> +++ b/net/netfilter/nfnetlink_acct.c
>> @@ -32,18 +32,24 @@ static LIST_HEAD(nfnl_acct_list);
>>  struct nf_acct {
>>       atomic64_t              pkts;
>>       atomic64_t              bytes;
>> +     unsigned long           flags;
>>       struct list_head        head;
>>       atomic_t                refcnt;
>>       char                    name[NFACCT_NAME_MAX];
>>       struct rcu_head         rcu_head;
>> +     char                    data[0];
>>  };
>>
>> +#define NFACCT_F_QUOTA (NFACCT_F_QUOTA_PKTS | NFACCT_F_QUOTA_BYTES)
>> +
>>  static int
>>  nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>            const struct nlmsghdr *nlh, const struct nlattr * const tb[])
>>  {
>>       struct nf_acct *nfacct, *matching = NULL;
>>       char *acct_name;
>> +     unsigned int size = 0;
>> +     u32 flags = 0;
>>
>>       if (!tb[NFACCT_NAME])
>>               return -EINVAL;
>> @@ -68,15 +74,39 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>                       /* reset counters if you request a replacement. */
>>                       atomic64_set(&matching->pkts, 0);
>>                       atomic64_set(&matching->bytes, 0);
>> +                     smp_mb__before_clear_bit();
>> +                     /* reset overquota flag if quota is enabled. */
>> +                     if ((matching->flags & NFACCT_F_QUOTA))
>> +                             clear_bit(NFACCT_F_OVERQUOTA, &matching->flags);
>>                       return 0;
>>               }
>>               return -EBUSY;
>>       }
>>
>>       nfacct = kzalloc(sizeof(struct nf_acct), GFP_KERNEL);
>> +     if (tb[NFACCT_FLAGS]) {
>> +             flags = ntohl(nla_get_be32(tb[NFACCT_FLAGS]));
>> +             if (flags & ~NFACCT_F_QUOTA)
>> +                     return -EOPNOTSUPP;
>> +             if ((flags & NFACCT_F_QUOTA) == NFACCT_F_QUOTA)
>> +                     return -EINVAL;
>> +             if (flags & NFACCT_F_OVERQUOTA)
>> +                     return -EINVAL;
>> +
>> +             size += sizeof(u64);
>> +     }
>> +
>> +     nfacct = kzalloc(sizeof(struct nf_acct) + size, GFP_KERNEL);
>>       if (nfacct == NULL)
>>               return -ENOMEM;
>>
>> +     if (flags & NFACCT_F_QUOTA) {
>> +             u64 *quota = (u64 *)nfacct->data;
>> +
>> +             *quota = be64_to_cpu(nla_get_be64(tb[NFACCT_QUOTA]));
>> +             nfacct->flags = flags;
>> +     }
>> +
>>       strncpy(nfacct->name, nla_data(tb[NFACCT_NAME]), NFACCT_NAME_MAX);
>>
>>       if (tb[NFACCT_BYTES]) {
>> @@ -117,6 +147,10 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>       if (type == NFNL_MSG_ACCT_GET_CTRZERO) {
>>               pkts = atomic64_xchg(&acct->pkts, 0);
>>               bytes = atomic64_xchg(&acct->bytes, 0);
>> +             if (acct->flags & NFACCT_F_QUOTA) {
>> +                     smp_mb__before_clear_bit();
>                         ^------------------------^
>
> I think you have to move this just before the if branch.

I've been wondering about that while looking at your code. Why execute
a memory barrier if we aren't sure that clear_bit() will be called?
I'm thinking you have a valid reason that I don't see.

>
>> +                     clear_bit(NFACCT_F_OVERQUOTA, &acct->flags);
>> +             }
>>       } else {
>>               pkts = atomic64_read(&acct->pkts);
>>               bytes = atomic64_read(&acct->bytes);
>> @@ -125,7 +159,13 @@ nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>           nla_put_be64(skb, NFACCT_BYTES, cpu_to_be64(bytes)) ||
>>           nla_put_be32(skb, NFACCT_USE, htonl(atomic_read(&acct->refcnt))))
>>               goto nla_put_failure;
>> +     if (acct->flags & NFACCT_F_QUOTA) {
>> +             u64 *quota = (u64 *)acct->data;
>>
>> +             if (nla_put_be32(skb, NFACCT_FLAGS, htonl(acct->flags)) ||
>> +                 nla_put_be64(skb, NFACCT_QUOTA, cpu_to_be64(*quota)))
>> +                     goto nla_put_failure;
>> +     }
>>       nlmsg_end(skb, nlh);
>>       return skb->len;
>>
>> @@ -270,6 +310,8 @@ static const struct nla_policy nfnl_acct_policy[NFACCT_MAX+1] = {
>>       [NFACCT_NAME] = { .type = NLA_NUL_STRING, .len = NFACCT_NAME_MAX-1 },
>>       [NFACCT_BYTES] = { .type = NLA_U64 },
>>       [NFACCT_PKTS] = { .type = NLA_U64 },
>> +     [NFACCT_FLAGS] = { .type = NLA_U32 },
>> +     [NFACCT_QUOTA] = { .type = NLA_U64 },
>>  };
>>
>>  static const struct nfnl_callback nfnl_acct_cb[NFNL_MSG_ACCT_MAX] = {
>> @@ -336,6 +378,50 @@ void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct)
>>  }
>>  EXPORT_SYMBOL_GPL(nfnl_acct_update);
>>
>> +static void nfnl_overquota_report(struct nf_acct *nfacct)
>> +{
>> +     int ret;
>> +     struct sk_buff *skb;
>> +
>> +     skb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
>> +     if (skb == NULL)
>> +             return;
>> +
>> +     ret = nfnl_acct_fill_info(skb, 0, 0, NFNL_MSG_ACCT_OVERQUOTA, 0,
>> +                               nfacct);
>> +     if (ret <= 0) {
>> +             kfree_skb(skb);
>> +             return;
>> +     }
>> +     netlink_broadcast(init_net.nfnl, skb, 0, NFNLGRP_ACCT_QUOTA,
>> +                       GFP_ATOMIC);
>> +}
>> +
>> +int nfnl_acct_overquota(const struct sk_buff *skb, struct nf_acct *nfacct)
>> +{
>> +     u64 now;
>> +     u64 *quota;
>> +     bool ret = NFACCT_UNDERQUOTA;
>
> enums are u32, so you have to use int there at least.

Of course - the side effects of doing things hastily.

>
>> +     /* no place here if we don't have a quota */
>> +     if (!(nfacct->flags & NFACCT_F_QUOTA))
>> +             return NFACCT_NO_QUOTA;
>> +
>> +     quota = (u64 *)nfacct->data;
>> +     now = (nfacct->flags & NFACCT_F_QUOTA_PKTS) ?
>> +            atomic64_read(&nfacct->pkts) : atomic64_read(&nfacct->bytes);
>> +
>> +     ret = now > *quota;
>> +
>> +     if (now >= *quota &&
>
> This should be:
>
>         if (now > *quota &&
>
> I believe we report once we're at least 1 byte over quota, right?

I take quota limits to be inclusive.  As such if we set a limit of 50
packets, I expect the 50th packet to be allowed through but not the
51st one.  On the flip side there is no way to tell when that 51st
packet will arrive - it can potentially take a long time and as such
decided it was worthy to send the notification right away.

>
>> +         !test_and_set_bit(NFACCT_F_OVERQUOTA, &nfacct->flags)) {
>> +             nfnl_overquota_report(nfacct);
>> +     }
>> +
>> +     return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(nfnl_acct_overquota);
>> +
>>  static int __init nfnl_acct_init(void)
>>  {
>>       int ret;
>> diff --git a/net/netfilter/xt_nfacct.c b/net/netfilter/xt_nfacct.c
>> index b3be0ef..94b0f09 100644
>> --- a/net/netfilter/xt_nfacct.c
>> +++ b/net/netfilter/xt_nfacct.c
>> @@ -21,10 +21,16 @@ MODULE_ALIAS("ip6t_nfacct");
>>
>>  static bool nfacct_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>  {
>> +     bool overquota;
>
> this needs to be convert to enum nfacct_quota_state or simply int as
> it is not a boolean anymore.

Evidently yes.

>
>>       const struct xt_nfacct_match_info *info = par->targinfo;
>>
>>       nfnl_acct_update(skb, info->nfacct);
>>
>> +     overquota = nfnl_acct_overquota(skb, info->nfacct);
>> +
>> +     if (overquota == NFACCT_UNDERQUOTA)
>> +             return false;
>> +
>>       return true;
>
> We can save a couple of lines with this:
>
>         quota_state = nfnl_acct_overquota(skb, info->nfacct);
>
>         return quota_state == NFACCT_UNDERQUOTA ? false : true;

ack

>
> It would be also great if you look at the nfacct tool extensions. I'd
> like to have a monitor mode that spots quota warnings for testing
> this.

I have most of the nfacct changes done but didn't send the patches as
it wasn't ready for submission yet.  Once we agreed on the kernel side
I was going to send the userspace.  Ok for the monitoring mode, I
already have a test application that does that.

Where will you want the patches to be sent?  Netdev and netfilter-dev
even if they are userspace?

>
> Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Netfitler Users]     [LARTC]     [Bugtraq]     [Yosemite Forum]

  Powered by Linux