Re: [PATCH 1/2] netfilter: add extended accounting infrastructure over nfnetlink

[Pablo Neira Ayuso <pablo@netfilter.org>]

On Fri, Dec 23, 2011 at 10:54:47PM +0800, Changli Gao wrote:
> On Fri, Dec 23, 2011 at 9:42 PM,  <pablo@netfilter.org> wrote:
> > +
> > +static LIST_HEAD(nfnl_acct_list);
> 
> You suppose that there won't be many accounting instants. It isn't
> scalable, as we have to iterate the list to find a special item. How
> about a hash table?

The lookup only happens if the rule is loaded.

There's no real gain from using hashing here.

> > +static int
> > +nfnl_acct_dump(struct sk_buff *skb, struct netlink_callback *cb)
> > +{
> > +       struct nf_acct *cur, *last;
> > +
> > +       if (cb->args[2])
> > +               return 0;
> > +
> > +       last = (struct nf_acct *)cb->args[1];
> > +       if (cb->args[1])
> > +               cb->args[1] = 0;
> > +
> > +       rcu_read_lock();
> > +       list_for_each_entry(cur, &nfnl_acct_list, head) {
> 
> You should use the RCU variant here.

I'll fix this, thanks.

> > +               if (last && cur != last)
> > +                       continue;
> > +
> > +               if (nfnl_acct_fill_info(skb, NETLINK_CB(cb->skb).pid,
> > +                                      cb->nlh->nlmsg_seq,
> > +                                      NFNL_MSG_ACCT_NEW, cur) < 0) {
> > +                       cb->args[1] = (unsigned long)cur;
> > +                       break;
> > +               }
> > +
> > +               if (NFNL_MSG_TYPE(cb->nlh->nlmsg_type) ==
> > +                                               NFNL_MSG_ACCT_GET_CTRZERO) {
> > +                       atomic64_set(&cur->pkts, 0);
> > +                       atomic64_set(&cur->bytes, 0);
> 
> The get and zero operations should be done in an atomic context,
> otherwise counters added between them will be lost.

Good catch.

I think we can fix this with xchg.

Similar patch should go to nf_conntrack_netlink.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html