possible ICMP accounting issue

possible ICMP accounting issue

[Joubert Berger]

I have been working on a very simple accounting package that I need and
ran into something that might be a small problem.  It deals with the reply
packet for ICMP requests.  They don't get added to the byte and packet
 counts:

In the icmp_packet() function, we have:
        if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY) {
                if (atomic_dec_and_test(&ct->proto.icmp.count)
                    && del_timer(&ct->timeout))
                         ct->timeout.function((unsigned long)ct);
        } else {
                atomic_inc(&ct->proto.icmp.count);
                nf_conntrack_event_cache(IPCT
_PROTOINFO_VOLATILE, skb);
                 nf_ct_refresh_acct(ct, ctinfo, skb, nf_ct_icmp_timeout);
        }
As you can see, the nf_ct_refresh_acct() is not called when it is a reply.
Presumably because the connection tracking entry's timers are being
 deleted and the entry is getting ready to be flushed anyway (and of course you
don't need to update timer if you are getting ready to remove it).

But, the way I implemented my accounting package's reporting, I do it
at the time that the
 connection tracking "destroy" function is called.  So, this means
that the reply packet is
never included in my counts.

I was going to add (plus appropriate locking):
                ct->counters[CTINFO2DIR(ctinfo)].packets++;
                 ct->counters[CTINFO2DIR(ctinfo)].bytes +=
                        skb->len - skb_network_offset(skb);

Is this really an issue or is it just a problem for me, because of
where I am collecting the
 accounting information?

If it is a problem, I can provide a patch.

--joubert

Re: possible ICMP accounting issue

[Patrick McHardy]

Joubert Berger wrote:
> I have been working on a very simple accounting package that I need and
> ran into something that might be a small problem.  It deals with the reply
> packet for ICMP requests.  They don't get added to the byte and packet
>  counts:
> 
> In the icmp_packet() function, we have:
>         if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY) {
>                 if (atomic_dec_and_test(&ct->proto.icmp.count)
>                     && del_timer(&ct->timeout))
>                          ct->timeout.function((unsigned long)ct);
>         } else {
>                 atomic_inc(&ct->proto.icmp.count);
>                 nf_conntrack_event_cache(IPCT
> _PROTOINFO_VOLATILE, skb);
>                  nf_ct_refresh_acct(ct, ctinfo, skb, nf_ct_icmp_timeout);
>         }
> As you can see, the nf_ct_refresh_acct() is not called when it is a reply.
> Presumably because the connection tracking entry's timers are being
>  deleted and the entry is getting ready to be flushed anyway (and of course you
> don't need to update timer if you are getting ready to remove it).
> 
> But, the way I implemented my accounting package's reporting, I do it
> at the time that the
>  connection tracking "destroy" function is called.  So, this means
> that the reply packet is
> never included in my counts.
> 
> I was going to add (plus appropriate locking):
>                 ct->counters[CTINFO2DIR(ctinfo)].packets++;
>                  ct->counters[CTINFO2DIR(ctinfo)].bytes +=
>                         skb->len - skb_network_offset(skb);
> 
> Is this really an issue or is it just a problem for me, because of
> where I am collecting the
>  accounting information?


I guess we need a new nf_ct_acct() function and should call
this before destruction (similar for TCP RST).