Re: [PATCH nf-next] netfilter: ctnetlink: don't dump ct extensions of unconfirmed conntracks

[Jeremy Sowden <jeremy@azazel.net>]

[-- Attachment #1: Type: text/plain, Size: 3731 bytes --]

On 2019-10-15, at 23:22:04 +0200, Florian Westphal wrote:
> Jeremy Sowden wrote:
> > On 2019-10-14, at 21:41:41 +0200, Florian Westphal wrote:
> > > When dumping the unconfirmed lists, the cpu that is processing the
> > > ct entry can realloc ct->ext at any time.
> > >
> > > Accessing extensions from another CPU is ok provided rcu read lock
> > > is held.
> > >
> > > Once extension space will be reallocated with plain krealloc this
> > > isn't used anymore.
> > >
> > > Dumping the extension area for confirmed or dying conntracks is
> > > fine: no reallocations are allowed and list iteration holds
> > > appropriate locks that prevent ct (and thus ct->ext) from getting
> > > free'd.
> > >
> > > Signed-off-by: Florian Westphal <fw@strlen.de>
> > > ---
> > >  net/netfilter/nf_conntrack_netlink.c | 77 ++++++++++++++++++----------
> > >  1 file changed, 51 insertions(+), 26 deletions(-)
> > >
> > > diff --git a/net/netfilter/nf_conntrack_netlink.c b/net/netfilter/nf_conntrack_netlink.c
> > > index e2d13cd18875..db04e1bfb04d 100644
> > > --- a/net/netfilter/nf_conntrack_netlink.c
> > > +++ b/net/netfilter/nf_conntrack_netlink.c
> > > @@ -506,9 +506,44 @@ static int ctnetlink_dump_use(struct sk_buff *skb, const struct nf_conn *ct)
> > >  	return -1;
> > >  }
> > >
> > > +/* all these functions access ct->ext. Caller must either hold a reference
> > > + * on ct or prevent its deletion by holding either the bucket spinlock or
> > > + * pcpu dying list lock.
> > > + */
> > > +static int ctnetlink_dump_extinfo(struct sk_buff *skb,
> > > +				  const struct nf_conn *ct, u32 type)
> > > +{
> > > +	if (ctnetlink_dump_acct(skb, ct, type) < 0 ||
> > > +	    ctnetlink_dump_timestamp(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_helpinfo(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_labels(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_ct_seq_adj(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_ct_synproxy(skb, ct) < 0)
> > > +		return -1;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int ctnetlink_dump_info(struct sk_buff *skb, struct nf_conn *ct)
> > > +{
> > > +	if (ctnetlink_dump_status(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_mark(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_secctx(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_id(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_use(skb, ct) < 0 ||
> > > +	    ctnetlink_dump_master(skb, ct) < 0)
> > > +		return -1;
> > > +
> > > +	if (!test_bit(IPS_OFFLOAD_BIT, &ct->status) &&
> > > +	    (ctnetlink_dump_timeout(skb, ct) < 0 ||
> > > +	     ctnetlink_dump_protoinfo(skb, ct) < 0))
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int
> > >  ctnetlink_fill_info(struct sk_buff *skb, u32 portid, u32 seq, u32 type,
> > > -		    struct nf_conn *ct)
> > > +		    struct nf_conn *ct, bool extinfo)
>
> [..]
>
> > > +
> > > +			/* We can't dump extension info for the unconfirmed
> > > +			 * list because unconfirmed conntracks can have ct->ext
> > > +			 * reallocated (and thus freed).
> > > +			 *
> > > +			 * In the dying list case ct->ext can't be altered during
> > > +			 * list walk anymore, and free can only occur after ct
> > > +			 * has been unlinked from the dying list (which can't
> > > +			 * happen until after we drop pcpu->lock).
> > > +			 */
> > >  			res = ctnetlink_fill_info(skb, NETLINK_CB(cb->skb).portid,
> > >  						  cb->nlh->nlmsg_seq,
> > >  						  NFNL_MSG_TYPE(cb->nlh->nlmsg_type),
> > > -						  ct);
> > > -			rcu_read_unlock();
> > > +						  ct, dying ? true : false);
> >
> > s/dying ? true : false/dying/
>
> Yes, but it found it misleading since the last argument isn't about
> 'dying' or not, it tells that we can safely access ct->ext.

Fair enough.  Read in the context of a patch, it just looks
tautological.

J.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]