Re: [PATCH nf v3 1/1] netfilter: nf_conntrack: defer invalid log until after unlock

Zihan Xi <[email protected]> wrote:
> TCP and SCTP conntrack paths can emit invalid-packet logs while ct->lock
> is still held.
> 
> When invalid logging is routed to nfnetlink_log and conntrack export is
> enabled, the log path can re-enter conntrack netlink glue and dump the
> same conntrack again. Protocol attribute dumping may take ct->lock, so
> logging while holding that lock can deadlock.
> 
> Defer the TCP invalid logs by storing only the minimal log context while
> ct->lock is held and emitting the log after unlocking. Also make the TCP
> timeout-lowering invalid path return whether a log is needed, then emit
> that log after unlocking.
> 
> Do the same for the SCTP invalid state-transition log that can be reached
> while ct->lock is held.
> 
> Add a lockdep assertion to nf_ct_l4proto_log_invalid() so future callers
> that log invalid conntracks while holding ct->lock are caught outside TCP
> and SCTP as well.
> 
> Fixes: 628d694344a0 ("netfilter: conntrack: reduce timeout when receiving out-of-window fin or rst")
> Fixes: d9a6f0d0df18 ("netfilter: conntrack: prepare tcp_in_window for ternary return value")
> Fixes: f71cb8f45d09 ("netfilter: conntrack: sctp: use nf log infrastructure for invalid packets")
> Cc: [email protected]
> Reported-by: Vega <[email protected]>
> Assisted-by: Codex:gpt-5.4
> Signed-off-by: Zihan Xi <[email protected]>
> Reviewed-by: Florian Westphal <[email protected]>
> ---
> changes in v3:
>   - Squash the previous two TCP patches into one patch as requested.
>   - Include the SCTP invalid state-transition log in the same logical fix.
>   - Move the lockdep assertion and recursion comment into nf_ct_l4proto_log_invalid().
>   - Adjust the TCP local declaration order per review.
>   - Use the broader nf_conntrack subject suggested by Pablo.
>   - Correct the Fixes: set to the first held-lock invalid-log call sites.
>   - Keep Florian Westphal's Reviewed-by.
>   - v2 Link: https://lore.kernel.org/all/[email protected]/
> changes in v2:
>   - Split the TCP timeout-lowering and tcp_in_window() handling for review.
>   - Switch the reproducer documentation to the packetdrill wrapper flow.
>   - Keep the decoded crash log and real reproduction artifacts in the cover letter.
>   - v1 Link: https://lore.kernel.org/all/[email protected]/
> ---
>  net/netfilter/nf_conntrack_proto.c      |   6 ++
>  net/netfilter/nf_conntrack_proto_sctp.c |  12 ++-
>  net/netfilter/nf_conntrack_proto_tcp.c  | 132 ++++++++++++++++--------
>  3 files changed, 102 insertions(+), 48 deletions(-)
> 
> diff --git a/net/netfilter/nf_conntrack_proto.c b/net/netfilter/nf_conntrack_proto.c
> index ad96896516b6..9439596f34e6 100644
> --- a/net/netfilter/nf_conntrack_proto.c
> +++ b/net/netfilter/nf_conntrack_proto.c
> @@ -83,6 +83,12 @@ void nf_ct_l4proto_log_invalid(const struct sk_buff *skb,
>  	if (likely(net->ct.sysctl_log_invalid == 0))
>  		return;
>  
> +	/* nfnetlink_log may re-enter conntrack attribute dumping and try to
> +	 * take ct->lock again via helpers such as tcp_to_nlattr(), so invalid
> +	 * conntrack logs must only be emitted after dropping ct->lock.
> +	 */
> +	lockdep_assert_not_held(&ct->lock);
> +

This should be unconditional, i.e. *before* the sysctl check.
Other than this, this fix LGTM.