Re: [PATCH v2 1/1] nf_conntrack_ftp: Added nfct_seqadj_ext_add() for ftp's conntrack.

[Florian Westphal <fw@strlen.de>]

Andrii Melnychenko <a.melnychenko@vyos.io> wrote:
> I've researched the issue a bit. Despite the fact that in `nf_nat_ftp()`
> the helper for the expected connection is installed, it isn't executed in
> the following functions - `nf_nat_mangle_tcp_packet()`. Also, shouldn't the
> logic of `nf_nat_follow_master` affect the "upcoming" passive FTP
> connection?

Yes, but we need the seqadj extension on the control connection to
rewrite the announced address to connect to/from.

nf_nat_setup_info() takes care of this but only for template-based
helper assignment, not for the explicit assign done via
nft_ct_helper_obj_eval().

> I've also checked the setup of `nfct_seqadj_ext_add()` in the
> `ft_ct_helper_obj_eval()` routine - it works. However, now the seqadj would
> be added to all "NATed" conntrack helpers.

Yes.

> Maybe it's better to leave the
> seqadj setup in `nf_conntrack_ftp`, so it would apply explicitly to FTP
> traffic, but with an additional `(ct->status & IPS_NAT_MASK)` check?

As-is, almost all the helpers are broken when used with nat and assignment
via nft objref infra.  We could add some annotation to those that don't
need seqadj, but afaics thats just the netbios helper.

> I can prepare a new patch with changes in either `nft_ct` or
> `nf_conntrack_ftp`.
> Any suggestions?

Thanks, please fix nft_ct infra.  Does the above make sense to you?