Hello,
On Tue, Jun 12, 2018 at 7:23 PM, Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> On Tue, Jun 12, 2018 at 03:21:35PM +0200, Florian Westphal wrote:
>> Harsha Sharma <harshasharmaiitr@xxxxxxxxx> wrote:
>> > +ctnl_timeout_parse_policy(void *timeouts,
>> > + const struct nf_conntrack_l4proto *l4proto,
>> > + struct net *net, const struct nlattr *attr)
>> > +{
>> > + int ret = 0;
>> > + struct nlattr **tb;
>> > +
>> > + if (!l4proto->ctnl_timeout.nlattr_to_obj)
>> > + return 0;
>>
>> You'll need to warp this with
>> #if IS_ENABLED(CONFIG_NF_CT_NETLINK_TIMEOUT)
>>
>> else this fails to build with CONFIG_NF_CT_NETLINK_TIMEOUT=n.
>
> That's a short term solution, yes. But we need native interface for
> this that integrates with the nft interface, let's talk about this
> during the workshop.
>
>> > +static void nft_ct_timeout_obj_eval(struct nft_object *obj,
>> > + struct nft_regs *regs,
>> > + const struct nft_pktinfo *pkt)
>> > +{
>> > + const struct nft_ct_timeout_obj *priv = nft_obj_data(obj);
>> > + struct ctnl_timeout *to_assign = NULL;
>> > + struct nf_conn_timeout *timeout_ext;
>> > + struct sk_buff *skb = pkt->skb;
>> > +
>> > + if (!priv->tmpl ||
>>
>> Better let nft_ct_timeout_obj_init() fail so priv->tmpl is always set.
>>
>> > + nf_ct_is_confirmed(priv->tmpl))
>> > + return;
>>
>> Uncessery, the template cannot be confirmed (else bug).
>>
>> > + to_assign = priv->timeout;
>> > +
>> > + nf_ct_set(skb, priv->tmpl, IP_CT_NEW);
>>
>> You will need to check that no previous conntrack exists here.
>>
>> > + timeout_ext = nf_ct_timeout_ext_add(priv->tmpl, priv->timeout, GFP_ATOMIC);
>>
>> This looks strange. The timeout extension either has to be attached to
>> the template (but then this has to happen in init, not here).
>>
>> > + if (timeout_ext) {
>> > + rcu_assign_pointer(timeout_ext->timeout, to_assign);
>> > + __set_bit(IPS_CONFIRMED_BIT, &priv->tmpl->status);
>>
>> Hmm, no, CONFIRMED means conntrack was inserted into the hash table,
>> this must never happen for templates.
>
> Harsha, please address comments from Florian.
Yes, sure. Thanks for the review.
> Regarding this one below...
>
>> Unrelated to your patch: I think timeout handling is braindead
>> in current conntrack, we should revisit this.
>
> By now, I think it's fine as is, I mean using the template, so
> Harsha/someone else can have a look at this in a second step.
>
> We'll need to update iptables too if we want to get rid of the
> template in any case.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
