Re: [PATCH 5/8] [PATCH] deliver events for conntracks created via ctnetlink

[Patrick McHardy <kaber@trash.net>]

Pablo Neira Ayuso wrote:
> As for now, the creation and update of conntracks via ctnetlink do not
> propagate an event to userspace. This can result in inconsistent situations
> if several userspace processes modify the connection tracking table by means
> of ctnetlink at the same time. Specifically, using the conntrack command
> line tool and conntrackd at the same time can trigger unconsistencies.
> 
> This patch also modifies the event cache infrastructure to pass the
> process PID and the ECHO flag to nfnetlink_send() to report back
> to userspace if the process that triggered the change needs so.
> Based on a suggestion from Patrick McHardy.

Looks pretty good, some minor issues:

- there are quite a lot of trailing whitespaces in this
   patch, please remove those.

> +/* This structure is passed to event handler */
> +struct nf_ct_event {
> +	struct nf_conn *ct;
> +	u32 pid;
> +	int report;
> +};

Just a suggestion: passing the nlmsghdr instead of the ECHO
flag and doing the approriate handling in the event functions
seems more logical to me. I think I know why you did it this
way (no reporting on unload, no netlink context there), see
below about that.

> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index f465090..aab2618 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -174,7 +174,8 @@ destroy_conntrack(struct nf_conntrack *nfct)
>  	NF_CT_ASSERT(atomic_read(&nfct->use) == 0);
>  	NF_CT_ASSERT(!timer_pending(&ct->timeout));
>  
> -	nf_conntrack_event(IPCT_DESTROY, ct);
> +	if (!test_bit(IPS_DYING_BIT, &ct->status))
> +		nf_conntrack_event(IPCT_DESTROY, ct);

Whats the idea behind this change? Is it simply an optimization?

>  	set_bit(IPS_DYING_BIT, &ct->status);
>  
>  	/* To make sure we don't get any weird locking issues here:
> @@ -963,8 +964,24 @@ void nf_ct_iterate_cleanup(struct net *net,
>  }
>  EXPORT_SYMBOL_GPL(nf_ct_iterate_cleanup);
>  
> +struct __nf_ct_flush_report {
> +	u32 pid;
> +	int report;
> +};
> +
>  static int kill_all(struct nf_conn *i, void *data)
>  {
> +	struct __nf_ct_flush_report *fr = (struct __nf_ct_flush_report *)data;
> +
> +	if (!fr->report)
> +		return 1;

Whats the reasoning behind not reporting destroy events on unload?
I don't think there's anything special about module unload, so it
seems inconsistent.