On Wed, Feb 26, 2020 at 11:58:04AM +0100, Pablo Neira Ayuso wrote:
> On Tue, Feb 25, 2020 at 09:58:47PM +0100, Pablo Neira Ayuso wrote:
> > On Tue, Feb 25, 2020 at 09:38:15PM +0100, Stefano Brivio wrote:
> > > On Tue, 25 Feb 2020 21:21:43 +0100
> > > Pablo Neira Ayuso <pablo@xxxxxxxxxxxxx> wrote:
> > >
> > > > Hi Stefano,
> > > >
> > > > On Tue, Feb 25, 2020 at 03:34:35PM +0100, Stefano Brivio wrote:
> > > > [...]
> > > > > This is the problem Phil reported:
> > > > [...]
> > > > > Or also simply with:
> > > > >
> > > > > # nft add element t s '{ 20-30 . 40 }'
> > > > > # nft add element t s '{ 25-35 . 40 }'
> > > > >
> > > > > the second element is silently ignored. I'm returning -EEXIST from
> > > > > nft_pipapo_insert(), but nft_add_set_elem() clears it because NLM_F_EXCL
> > > > > is not set.
> > > > >
> > > > > Are you suggesting that this is consistent and therefore not a problem?
> > > >
> > > > NLM_F_EXCL !NLM_F_EXCL
> > > > exact match EEXIST 0 [*]
> > > > partial match EEXIST EEXIST
> > > >
> > > > The [*] case would allow for element timeout/expiration updates from
> > > > the control plane for exact matches.
> > >
> > > A-ha. I didn't even consider that.
> > >
> > > > Note that element updates are not
> > > > supported yet, so this check for !NLM_F_EXCL is a stub. I don't think
> > > > we should allow for updates on partial matches
> > > >
> > > > I think what it is missing is a error to report "partial match" from
> > > > pipapo. Then, the core translates this "partial match" error to EEXIST
> > > > whether NLM_F_EXCL is set or not.
> > >
> > > Yes, given what you explained, I also think it's the case.
> > >
> > > > Would this work for you?
> > >
> > > It would. I need to write a few more lines in nft_pipapo_insert(),
> > > because right now I don't have a special case for "entirely
> > > overlapping". Something on the lines of:
> > >
> > > dup = pipapo_get(net, set, start, genmask);
> > > if (PTR_ERR(dup) == -ENOENT) {
> > >
> > > --> compare start and end key for this entry with
> > > start and end key from 'ext'
> > >
> > > Let me know if you want me to post a patch with a placeholder for
> > > whatever you have in mind, or if I can help implementing this, etc.
> >
> > Please, go ahead with the placeholder, it might be faster. I'll jump
> > on it.
>
> Sorry, I just realized I can be probably quicker on the core side.
> Here is my proposal.
>
> I'm attaching a patch for the core. This is handling -ENOTEMPTY which
> is (ab)used to report the partial element matching.
>
> if NLM_F_EXCL is set off, then -EEXIST becomes 0.
> then -ENOTEMPTY becomes -EEXIST.
>
> Would this work for you?
>
> Thanks.
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index d1318bdf49ca..0bbe36e731b8 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -5059,7 +5059,8 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
> ext->genmask = nft_genmask_cur(ctx->net) | NFT_SET_ELEM_BUSY_MASK;
> err = set->ops->insert(ctx->net, set, &elem, &ext2);
> if (err) {
> - if (err == -EEXIST) {
> + /* ENOTEMPTY reports partial element matching. */
> + if (err == -EEXIST || err == -ENOTEMPTY) {
> if (nft_set_ext_exists(ext, NFT_SET_EXT_DATA) ^
> nft_set_ext_exists(ext2, NFT_SET_EXT_DATA) ||
> nft_set_ext_exists(ext, NFT_SET_EXT_OBJREF) ^
> @@ -5075,8 +5076,12 @@ static int nft_add_set_elem(struct nft_ctx *ctx, struct nft_set *set,
> nft_set_ext_exists(ext2, NFT_SET_EXT_OBJREF) &&
> *nft_set_ext_obj(ext) != *nft_set_ext_obj(ext2)))
> err = -EBUSY;
> - else if (!(nlmsg_flags & NLM_F_EXCL))
> - err = 0;
> + else if (!(nlmsg_flags & NLM_F_EXCL)) {
> + if (err == -EEXIST)
> + err = 0;
> + else if (err == -ENOTEMPTY)
> + err = -EEXIST;
BTW, this could be simplified to:
if (err == -ENOTEMPTY)
err = -EEXIST;
else
err = 0;
instead of:
if (err == -EEXIST)
err = 0;
else if (err == -ENOTEMPTY)
err = -EEXIST;
> + }
> }
> goto err_element_clash;
> }
