Re: [PATCH v5] netfilter: xtables: add quota support for nfacct

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

 



I haven't heard back on this for 12 full days - is there anything
you'd like me to clarify?  If you are busy can you at least provide me
with a date by which I should expect an answer?

Mathieu

On 26 February 2014 11:38, Mathieu Poirier <mathieu.poirier@xxxxxxxxxx> wrote:
> On 25 February 2014 10:54, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
>> Hi Mathieu,
>>
>> This is looking better, but still more comments to address, see below.
>>
>> On Tue, Feb 18, 2014 at 09:20:02AM -0700, mathieu.poirier@xxxxxxxxxx wrote:
>>> From: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>>>
>>> Adding packet and byte quota support.  Once a quota has been
>>> reached a noticifaction is sent to user space that includes
>>> the name of the accounting object along with the current byte
>>> and packet count.
>>>
>>> Signed-off-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
>>> ---
>>> Changes for v5:
>>> - Removed spinlock from 'nfnl_acct_update'.
>>> - Added accounting object store.
>>> ---
>>>  include/linux/netfilter/nfnetlink_acct.h      |  17 ++-
>>>  include/uapi/linux/netfilter/nfnetlink.h      |   2 +
>>>  include/uapi/linux/netfilter/nfnetlink_acct.h |   1 +
>>>  include/uapi/linux/netfilter/xt_nfacct.h      |  18 ++-
>>>  net/netfilter/Kconfig                         |   3 +-
>>>  net/netfilter/nfnetlink_acct.c                |  46 +++++--
>>>  net/netfilter/xt_nfacct.c                     | 179 ++++++++++++++++++++++++--
>>>  7 files changed, 238 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/include/linux/netfilter/nfnetlink_acct.h b/include/linux/netfilter/nfnetlink_acct.h
>>> index b2e85e5..bb04204 100644
>>> --- a/include/linux/netfilter/nfnetlink_acct.h
>>> +++ b/include/linux/netfilter/nfnetlink_acct.h
>>> @@ -3,11 +3,24 @@
>>>
>>>  #include <uapi/linux/netfilter/nfnetlink_acct.h>
>>>
>>> +struct nf_acct {
>>> +     atomic64_t              pkts;
>>> +     atomic64_t              bytes;
>>> +     struct list_head        head;
>>> +     atomic_t                refcnt;
>>> +     char                    name[NFACCT_NAME_MAX];
>>> +     struct rcu_head         rcu_head;
>>> +};
>>>
>>> -struct nf_acct;
>>> +enum nfnl_acct_udt_type {
>>> +     NFNL_ACCT_UDT_PACKETS,
>>> +     NFNL_ACCT_UDT_BYTES,
>>> +};
>>
>> I prefer some like this instead of the one above:
>>
>> enum nfacct_quota_type {
>>         NFACCT_QUOTA_PACKETS = 0,
>>         NFACCT_QUOTA_BYTES
>> };
>>
>>>  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 u64 nfnl_acct_update(const struct sk_buff *skb,
>>> +                        struct nf_acct *nfacct, int mode);
>>> +void nfnl_quota_event(struct nf_acct *cur);
>>>
>>>  #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..ae8ea0a 100644
>>> --- a/include/uapi/linux/netfilter/nfnetlink_acct.h
>>> +++ b/include/uapi/linux/netfilter/nfnetlink_acct.h
>>> @@ -19,6 +19,7 @@ enum nfnl_acct_type {
>>>       NFACCT_PKTS,
>>>       NFACCT_BYTES,
>>>       NFACCT_USE,
>>> +     NFACCT_QUOTA,
>>
>> This is wrong. You are using this new attribute (which defines another
>> TLV in the payload of a message) instead of the netlink message type
>> in nfnl_acct_fill_info(). This needs a specific message type to
>> announce that the quota has been exceeded.
>
> If I understand you correctly you want a new entry in
> "nfnl_acct_msg_types" - please confirm.
>
>>
>>>       __NFACCT_MAX
>>>  };
>>>  #define NFACCT_MAX (__NFACCT_MAX - 1)
>>> diff --git a/include/uapi/linux/netfilter/xt_nfacct.h b/include/uapi/linux/netfilter/xt_nfacct.h
>>> index 3e19c8a..7c35ce0 100644
>>> --- a/include/uapi/linux/netfilter/xt_nfacct.h
>>> +++ b/include/uapi/linux/netfilter/xt_nfacct.h
>>> @@ -3,11 +3,27 @@
>>>
>>>  #include <linux/netfilter/nfnetlink_acct.h>
>>>
>>> +enum xt_quota_flags {
>>> +     XT_NFACCT_QUOTA_PKTS    = 1 << 0,
>>> +     XT_NFACCT_QUOTA         = 1 << 1,
>>> +};
>>> +
>>>  struct nf_acct;
>>> +struct nf_acct_quota;
>>>
>>>  struct xt_nfacct_match_info {
>>>       char            name[NFACCT_NAME_MAX];
>>> -     struct nf_acct  *nfacct;
>>> +     struct nf_acct  *nfacct __aligned(8);
>>>  };
>>>
>>> +struct xt_nfacct_match_info_v1 {
>>> +     char            name[NFACCT_NAME_MAX];
>>> +     struct nf_acct  *nfacct __aligned(8);
>>
>> You have to move at the bottom of the structure, before the new
>> nf_acct_quota.
>>
>> The extension from userspace sets the .userspacesize field, which
>> helps iptables to skip internal pointers that are set by the kernel,
>> If you don't fix this, iptables -D to delete a rule with nfacct will
>> not work.
>
> Thanks for pointing that out - I've been experiencing problems with
> "iptables -D".
>
>>
>>> +     __u32 flags;
>>> +     __aligned_u64 quota;
>>> +
>>> +     /* used internally by kernel */
>>> +     struct nf_acct_quota *priv __aligned(8);
>>> +};
>>>  #endif /* _XT_NFACCT_MATCH_H */
>>> diff --git a/net/netfilter/Kconfig b/net/netfilter/Kconfig
>>> index 748f304..ce184951 100644
>>> --- a/net/netfilter/Kconfig
>>> +++ b/net/netfilter/Kconfig
>>> @@ -1108,7 +1108,8 @@ config NETFILTER_XT_MATCH_NFACCT
>>>       select NETFILTER_NETLINK_ACCT
>>>       help
>>>         This option allows you to use the extended accounting through
>>> -       nfnetlink_acct.
>>> +       nfnetlink_acct.  It is also possible to add quotas at the
>>> +       packet and byte level.
>>>
>>>         To compile it as a module, choose M here.  If unsure, say N.
>>>
>>> diff --git a/net/netfilter/nfnetlink_acct.c b/net/netfilter/nfnetlink_acct.c
>>> index c7b6d46..5c580a7 100644
>>> --- a/net/netfilter/nfnetlink_acct.c
>>> +++ b/net/netfilter/nfnetlink_acct.c
>>> @@ -29,15 +29,6 @@ MODULE_DESCRIPTION("nfacct: Extended Netfilter accounting infrastructure");
>>>
>>>  static LIST_HEAD(nfnl_acct_list);
>>>
>>> -struct nf_acct {
>>> -     atomic64_t              pkts;
>>> -     atomic64_t              bytes;
>>> -     struct list_head        head;
>>> -     atomic_t                refcnt;
>>> -     char                    name[NFACCT_NAME_MAX];
>>> -     struct rcu_head         rcu_head;
>>> -};
>>> -
>>>  static int
>>>  nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>>            const struct nlmsghdr *nlh, const struct nlattr * const tb[])
>>> @@ -92,7 +83,7 @@ nfnl_acct_new(struct sock *nfnl, struct sk_buff *skb,
>>>       return 0;
>>>  }
>>>
>>> -static int
>>> +int
>>>  nfnl_acct_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
>>>                  int event, struct nf_acct *acct)
>>>  {
>>> @@ -134,6 +125,7 @@ nla_put_failure:
>>>       nlmsg_cancel(skb, nlh);
>>>       return -1;
>>>  }
>>> +EXPORT_SYMBOL_GPL(nfnl_acct_fill_info);
>>
>> I think you don't need to export this symbol.
>>
>>>  static int
>>>  nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
>>> @@ -329,13 +321,41 @@ void nfnl_acct_put(struct nf_acct *acct)
>>>  }
>>>  EXPORT_SYMBOL_GPL(nfnl_acct_put);
>>>
>>> -void nfnl_acct_update(const struct sk_buff *skb, struct nf_acct *nfacct)
>>> +u64 nfnl_acct_update(const struct sk_buff *skb,
>>> +                 struct nf_acct *nfacct, int mode)
>>>  {
>>> -     atomic64_inc(&nfacct->pkts);
>>> -     atomic64_add(skb->len, &nfacct->bytes);
>>> +     u64 pkts, bytes;
>>> +
>>> +     pkts = atomic64_inc_return(&nfacct->pkts);
>>> +     bytes = atomic64_add_return(skb->len, &nfacct->bytes);
>>> +
>>> +     return (mode == NFNL_ACCT_UDT_PACKETS) ? pkts : bytes;
>>>  }
>>>  EXPORT_SYMBOL_GPL(nfnl_acct_update);
>>>
>>> +void
>>> +nfnl_quota_event(struct nf_acct *cur)
>>> +{
>>> +     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, NFACCT_QUOTA,
>>> +                              NFNL_MSG_ACCT_NEW, cur);
>>> +
>>
>> Remove empty line above.
>>
>>> +     if (ret <= 0) {
>>> +             kfree_skb(skb);
>>> +             return;
>>> +     }
>>> +
>>> +     netlink_broadcast(init_net.nfnl, skb, 0,
>>> +                      NFNLGRP_ACCT_QUOTA, GFP_ATOMIC);
>>> +}
>>> +EXPORT_SYMBOL_GPL(nfnl_quota_event);
>>> +
>>>  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..d1bd5ea 100644
>>> --- a/net/netfilter/xt_nfacct.c
>>> +++ b/net/netfilter/xt_nfacct.c
>>> @@ -9,8 +9,10 @@
>>>  #include <linux/module.h>
>>>  #include <linux/skbuff.h>
>>>
>>> +#include <net/netlink.h>
>>>  #include <linux/netfilter/x_tables.h>
>>>  #include <linux/netfilter/nfnetlink_acct.h>
>>> +#include <linux/netfilter/nfnetlink.h>
>>>  #include <linux/netfilter/xt_nfacct.h>
>>>
>>>  MODULE_AUTHOR("Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx>");
>>> @@ -19,15 +21,62 @@ MODULE_LICENSE("GPL");
>>>  MODULE_ALIAS("ipt_nfacct");
>>>  MODULE_ALIAS("ip6t_nfacct");
>>>
>>> +static LIST_HEAD(nfacct_list);
>>> +static DEFINE_MUTEX(nfacct_list_mutex);
>>> +
>>> +struct nf_acct_quota {
>>> +     char                    name[NFACCT_NAME_MAX];
>>> +     int                     count;
>>> +     u32                     flags;
>>> +     u64                     quota;
>>> +     struct list_head        node;
>>> +     atomic_t                notified;
>>> +};
>>> +
>>>  static bool nfacct_mt(const struct sk_buff *skb, struct xt_action_param *par)
>>>  {
>>>       const struct xt_nfacct_match_info *info = par->targinfo;
>>> +     u64 __always_unused val;
>>> -     nfnl_acct_update(skb, info->nfacct);
>>> +     val = nfnl_acct_update(skb, info->nfacct, 0);
>>
>> I think you can just do:
>>
>>         nfnl_acct_update(skb, info->nfacct...);
>>
>> and ignore the returned value in this case.
>>
>>>       return true;
>>>  }
>>>
>>> +static bool
>>> +nfacct_mt_v1(const struct sk_buff *skb, struct xt_action_param *par)
>>> +{
>>> +     int mode;
>>> +     u64 val;
>>> +     const struct xt_nfacct_match_info_v1 *info = par->matchinfo;
>>> +     struct nf_acct_quota *acct_quota = info->priv;
>>> +     bool ret = true;
>>> +
>>> +     mode = (acct_quota->flags & XT_NFACCT_QUOTA_PKTS) ?
>>> +            NFNL_ACCT_UDT_PACKETS : NFNL_ACCT_UDT_BYTES;
>>> +
>>> +     /* upgrade accounting stats */
>>> +     val = nfnl_acct_update(skb, info->nfacct, mode);
>>> +
>>> +     /* no need to go further if we don't have a quota */
>>> +     if (!(acct_quota->flags & XT_NFACCT_QUOTA))
>>> +             return ret;
>>> +
>>> +     /* are we over the limit ? */
>>> +     if (val <= info->quota) {
>>
>> You use <=, I think this should be <. Not further below you're using >=.
>
> That is the intended purpose - I take the limit to be inclusive.  If
> you have a limit of 200 packets, you want the packet that makes the
> count go from 199 to 200 to be allowed through.  At the same time you
> want the the notification to be sent out right away since you don't
> know how long it will take to receive the next packet.  Get back to me
> on that if you don't agree with my logic.
>
>>
>>> +             /* reset flag in case userspace cleared the counters */
>>> +             atomic_set(&acct_quota->notified, 0)
>>
>> This has runtime overhead and I think it's racy.
>
> Good catch on the racy part.
>
>  I think we can avoid
>> this if the quota object management is moved to nfnetlink_acct (see
>> below for further explanation). The idea is to iterate over the list
>> of quota objects (to find notified flags that need to be reset) by
>> when NFNL_MSG_ACCT_GET_CTRZERO is called.
>>
>>> +             ret = !ret;
>>> +     }
>>> +
>>> +     if (val >= info->quota)
>>
>> You have to use brackets here:
>>
>>         if (...) {
>>                 ...
>>         }
>>
>>> +             /* cmpxchg guarantees only one CPU can send a notification */
>>> +             if (atomic_cmpxchg(&acct_quota->notified, 0, 1) == 0)
>>> +                     nfnl_quota_event(info->nfacct);
>>> +
>>> +     return ret;
>>> +}
>>> +
>>>  static int
>>>  nfacct_mt_checkentry(const struct xt_mtchk_param *par)
>>>  {
>>> @@ -44,32 +93,140 @@ nfacct_mt_checkentry(const struct xt_mtchk_param *par)
>>>       return 0;
>>>  }
>>>
>>> +static
>>> +struct nf_acct_quota *nfacct_find_quota(char *name)
>>> +{
>>> +     struct nf_acct_quota *curr, *match = NULL;
>>> +
>>> +     mutex_lock(&nfacct_list_mutex);
>>> +     list_for_each_entry(curr, &nfacct_list, node)
>>
>> missing brackets here in list_for_each_entry too.
>>
>>> +             if (strncmp(curr->name, name, NFACCT_NAME_MAX) == 0) {
>>> +                     match = curr;
>>> +                     match->count++;
>>> +                     break;
>>> +             }
>>> +
>>> +     mutex_unlock(&nfacct_list_mutex);
>>> +
>>> +     return match;
>>> +}
>>> +
>>> +static struct  nf_acct_quota *
>>> +nfacct_create_quota(struct xt_nfacct_match_info_v1 *info)
>>
>> Thinking of nftables and the reset notified flag above, I think it's
>> better if you move the quota object management to nfnetlink_acct.c by
>> adding new nfacct commands. The idea is to maintain a quota object
>> base that can be used by xt_nfacct. You have to add the following
>> commands:
>>
>> enum nfnl_acct_msg_types {
>>         ...
>>         NFNL_MSG_ACCT_QUOTA_NEW,
>>         NFNL_MSG_ACCT_QUOTA_GET,
>>         NFNL_MSG_ACCT_QUOTA_DEL,
>>         NFNL_MSG_ACCT_MAX
>> };
>>
>> Then, extend nfnl_callback to include this new commands. The idea is
>> to create/delete quota objects from the netlink interface, then attach
>> them from the iptables nfacct extension.
>>
>> You won't need the mutex anymore btw, since it's already protected by
>> nfnl_lock.
>
> Dealing with the overhead introduced by clearing the "notified" flag
> is completely different from the management of "quota" objects.  To do
> this the best solution is to introduce a callback to reset the flag
> when a "nfacct" object gets reset.  That would be simple and in line
> with what is used ubiquitously in the kernel.
>
> Moving the management of "quota" objects to nfnetlink_acct is a
> completely different ball game.  Not only that, it would also mandate
> to make changes to nfacct, libnfnl and libnl.  It would also require
> to pull a few tricks to make all that extra code compile conditionally
> when XT_QUOTA is not selected, unless we want to automatically select
> XT_QUOTA when NETLINK_ACCT gets enabled.  And what happens when
> another quota type-of-thing comes along?  We keep stuffing "nfacct"
> and "nfnetlink_acct.c" with more functionality?
>
> To me introducing a callback for each of the operation supported by
> "nfnl_acct_cb" is the way to go.  It would allow any future extension
> so take action whenever something happens to an "nfacct" object.
>
> I'll continue thinking about this but take my proposal into account
> and get back to me.
>
>>
>>> +{
>>> +     struct nf_acct_quota *acct_quota;
>>> +
>>> +     mutex_lock(&nfacct_list_mutex);
>>> +
>>> +     acct_quota = kmalloc(sizeof(struct nf_acct_quota), GFP_ATOMIC);
>>> +
>>
>> remove empty line above.
>>
>>> +     if (!acct_quota) {
>>> +             mutex_unlock(&nfacct_list_mutex);
>>> +             return NULL;
>>> +     }
>>> +
>>> +     strncpy(acct_quota->name, info->name, NFACCT_NAME_MAX);
>>> +     acct_quota->flags = info->flags;
>>> +     acct_quota->quota = info->quota;
>>> +     acct_quota->count = 1;
>>> +     atomic_set(&acct_quota->notified, 0);
>>> +     list_add_tail(&acct_quota->node, &nfacct_list);
>>
>> This needs to be converted to _rcu.
>
> You mean in the context that we move forward with your suggestion?
--
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