Re: [PATCH nf 1/1] netfilter: nf_conntrack_tcp: defer invalid logging until after unlock

Zihan Xi <[email protected]> wrote:
> nf_conntrack_tcp_packet() can log invalid packets while ct->lock is still
> held. This happens both through tcp_in_window() -> nf_tcp_log_invalid()
> and through the NFCT_TCP_INVALID path -> nf_tcp_handle_invalid().

Correct, this is a bug. While I like the general direction, I find this
patch too large and too complicated.

Please consider making at least two patches to fix this.

- One patch to make nf_tcp_handle_invalid() return bool, then log
outside of spinlock().  The timeout doesn't even have to be included,
	you can change the message to omit it (e.g. lowered timeout to
			UNACK or whatever).

This will help with review and it will simplify this patch too.

> +enum nf_tcp_invalid_log_type {
> +	NF_TCP_LOG_NONE,
> +	NF_TCP_LOG_OVERSHOT,
> +	NF_TCP_LOG_SEQ_OVER,
> +	NF_TCP_LOG_ACK_OVER,
> +	NF_TCP_LOG_SEQ_UNDER,
> +	NF_TCP_LOG_ACK_UNDER,

This is fine.

> +struct nf_tcp_invalid_log {
> +	enum nf_tcp_invalid_log_type type;
> +	u32 value;
> +	u8 index;
> +	u8 dir;
> +	u8 last_index;

index, dir, last_index no longer need this stashing once lowered-timeout
is solved as outlined above.

> +struct nf_tcp_invalid_logs {
> +	/* At most one tcp_in_window() log plus one lower-timeout log. */
> +	struct nf_tcp_invalid_log entries[2];

Same, once the lower-timeout is handled differently this is not needed.

> +static enum nf_ct_tcp_action
> +nf_tcp_log_invalid(const struct nf_conn *ct,
> +		   const struct ip_ct_tcp_state *sender,
> +		   struct nf_tcp_invalid_logs *logs,
> +		   enum nf_ct_tcp_action ret,
> +		   enum nf_tcp_invalid_log_type type,
> +		   u32 value)
>  {

This function doesn't log anymore.

>  static enum nf_ct_tcp_action
>  tcp_in_window(struct nf_conn *ct, enum ip_conntrack_dir dir,
>  	      unsigned int index, const struct sk_buff *skb,
>  	      unsigned int dataoff, const struct tcphdr *tcph,
> -	      const struct nf_hook_state *hook_state)
> +	      struct nf_tcp_invalid_logs *logs)
>  {
>  	struct ip_ct_tcp *state = &ct->proto.tcp;
>  	struct ip_ct_tcp_state *sender = &state->seen[dir];
> @@ -640,31 +728,29 @@ tcp_in_window(struct nf_conn *ct, enum ip_conntrack_dir dir,
>  			sender->td_end = end;
>  			sender->flags |= IP_CT_TCP_FLAG_DATA_UNACKNOWLEDGED;
>  
> -			return nf_tcp_log_invalid(skb, ct, hook_state, sender, NFCT_TCP_IGNORE,
> -						  "%u bytes more than expected", overshot);
> +			return nf_tcp_log_invalid(ct, sender, logs, NFCT_TCP_IGNORE,
> +					  NF_TCP_LOG_OVERSHOT, overshot);

Maybe rename to nf_tcp_store_invalid() or similar.  It only has to stash
the value (overshoot in this case) and NF_TCP_LOG_OVERSHOT.

Could you also add a lockdep_assert_not_held(ct->Lock) to the old
nf_ct_log_invalid() plus a comment explaining that we must not call into
nfnetlink_log with that lock held?

Thanks!