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

On Wed, Jul 29, 2026 at 10:59 PM Florian Westphal <[email protected]> wrote:
>
> 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!

Thanks, this makes sense. I’ll split the fix into at least two
patches, first handling the nf_tcp_handle_invalid() timeout-lowering
log separately by deferring it until after unlocking, then simplifying
the remaining tcp_in_window() invalid logging path and renaming the
helper accordingly. I’ll also add a lockdep assertion/comment to make
the no-log-under-ct->lock rule explicit in v2.