diff mbox series

netfilter: nf_tables: delete table via table handle

Message ID 20180106183015.11557-1-harshasharmaiitr@gmail.com
State Changes Requested
Delegated to: Pablo Neira
Headers show
Series netfilter: nf_tables: delete table via table handle | expand

Commit Message

Harsha Sharma Jan. 6, 2018, 6:30 p.m. UTC
This patch add code to delete table via unique table handle.

Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
---
 net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 92 insertions(+), 7 deletions(-)

Comments

Pablo Neira Ayuso Jan. 6, 2018, 11:02 p.m. UTC | #1
On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
> This patch add code to delete table via unique table handle.
> 
> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
> ---
>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 92 insertions(+), 7 deletions(-)
> 
> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> index dabdd2ed66c8..3b1c879fdf61 100644
> --- a/net/netfilter/nf_tables_api.c
> +++ b/net/netfilter/nf_tables_api.c
> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
>  	return NULL;
>  }
>  
> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
> +						      u64 handle)
> +{
> +	struct nft_af_info *afi;
> +	struct nft_table *table;
> +	int table_handle_check_flag = 0;
> +
> +	list_for_each_entry(afi, &net->nft.af_info, list) {
> +		list_for_each_entry(table, &afi->tables, list) {
> +			if (table->handle == handle)
> +				table_handle_check_flag = 1;

Use:
                                return table;

instead.

> +		}
> +		if (table_handle_check_flag)
> +			return afi;
> +	}
> +	return NULL;
> +}
> +
>  static struct nft_af_info *
>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>  {
> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>  	return ERR_PTR(-EAFNOSUPPORT);
>  }
>  
> +static struct nft_af_info *
> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
> +{
> +	struct nft_af_info *afi;
> +
> +	afi = nft_afinfo_lookup_byhandle(net, handle);
> +	if (afi != NULL)
> +		return afi;
> +#ifdef CONFIG_MODULES
> +	if (autoload) {
> +		nfnl_unlock(NFNL_SUBSYS_NFTABLES);
> +		request_module("nft-afinfo");
> +		nfnl_lock(NFNL_SUBSYS_NFTABLES);
> +		afi = nft_afinfo_lookup_byhandle(net, handle);
> +		if (afi != NULL)
> +			return ERR_PTR(-EAGAIN);
> +	}
> +#endif
> +	return ERR_PTR(-EAFNOSUPPORT);
> +}

I don't think you need this new nf_tables_afinfo_lookup_byhandle()
function. The handle parameter is never used. That will simplify your
patchset.

> +
>  static void nft_ctx_init(struct nft_ctx *ctx,
>  			 struct net *net,
>  			 const struct sk_buff *skb,
> @@ -107,7 +146,7 @@ static void nft_ctx_init(struct nft_ctx *ctx,
>  	ctx->afi	= afi;
>  	ctx->table	= table;
>  	ctx->chain	= chain;
> -	ctx->nla   	= nla;
> +	ctx->nla	= nla;
>  	ctx->portid	= NETLINK_CB(skb).portid;
>  	ctx->report	= nlmsg_report(nlh);
>  	ctx->seq	= nlh->nlmsg_seq;
> @@ -367,6 +406,28 @@ static struct nft_table *nft_table_lookup(const struct nft_af_info *afi,
>  	return NULL;
>  }
>  
> +static struct nft_table *__nft_table_lookup_byhandle(const struct nft_af_info *afi,
> +						     u64 handle, u8 genmask)
> +{
> +	struct nft_table *table;
> +
> +	list_for_each_entry(table, &afi->tables, list) {
> +		if (handle == table->handle &&
> +		    nft_active_genmask(table, genmask))
> +			return table;
> +	}
> +	return NULL;
> +}
> +
> +static struct nft_table *nft_table_lookup_byhandle(const struct nft_af_info *afi,
> +						   const struct nlattr *nla,
> +						   u8 genmask)
> +{
> +	return __nft_table_lookup_byhandle(afi,
> +					   be64_to_cpu(nla_get_be64(nla)),
> +					   genmask);
> +}
> +
>  static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
>  						const struct nlattr *nla,
>  						u8 genmask)
> @@ -383,6 +444,22 @@ static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
>  	return ERR_PTR(-ENOENT);
>  }
>  
> +static struct nft_table *nf_tables_table_lookup_byhandle(const struct nft_af_info *afi,
> +							 const struct nlattr *nla,
> +							 u8 genmask)
> +{
> +	struct nft_table *table;
> +
> +	if (nla == NULL)
> +		return ERR_PTR(-EINVAL);
> +
> +	table = nft_table_lookup_byhandle(afi, nla, genmask);
> +	if (table != NULL)
> +		return table;
> +
> +	return ERR_PTR(-ENOENT);
> +}
> +
>  static inline u64 nf_tables_alloc_handle(struct nft_table *table)
>  {
>  	return ++table->hgenerator;
> @@ -854,14 +931,22 @@ static int nf_tables_deltable(struct net *net, struct sock *nlsk,
>  	struct nft_ctx ctx;
>  
>  	nft_ctx_init(&ctx, net, skb, nlh, NULL, NULL, NULL, nla);
> -	if (family == AF_UNSPEC || nla[NFTA_TABLE_NAME] == NULL)
> +	if (nla[NFTA_TABLE_HANDLE]) {
> +		afi = nf_tables_afinfo_lookup_byhandle(net, be64_to_cpu(nla_get_be64(nla[NFTA_TABLE_HANDLE])), false);
> +		if (IS_ERR(afi))
> +			return PTR_ERR(afi);
> +		else
> +			table = nf_tables_table_lookup_byhandle(afi, nla[NFTA_TABLE_HANDLE], genmask);
> +	} else if (family == AF_UNSPEC || nla[NFTA_TABLE_NAME] == NULL) {
>  		return nft_flush(&ctx, family);
> +	} else {
>  
> -	afi = nf_tables_afinfo_lookup(net, family, false);
> -	if (IS_ERR(afi))
> -		return PTR_ERR(afi);
> -
> -	table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME], genmask);
> +		afi = nf_tables_afinfo_lookup(net, family, false);
> +		if (IS_ERR(afi))
> +			return PTR_ERR(afi);
> +		else
> +			table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME], genmask);
> +	}
>  	if (IS_ERR(table))
>  		return PTR_ERR(table);
>  
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Harsha Sharma Jan. 7, 2018, 9:19 a.m. UTC | #2
On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
>> This patch add code to delete table via unique table handle.
>>
>> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
>> ---
>>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
>>  1 file changed, 92 insertions(+), 7 deletions(-)
>>
>> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> index dabdd2ed66c8..3b1c879fdf61 100644
>> --- a/net/netfilter/nf_tables_api.c
>> +++ b/net/netfilter/nf_tables_api.c
>> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
>>       return NULL;
>>  }
>>
>> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
>> +                                                   u64 handle)
>> +{
>> +     struct nft_af_info *afi;
>> +     struct nft_table *table;
>> +     int table_handle_check_flag = 0;
>> +
>> +     list_for_each_entry(afi, &net->nft.af_info, list) {
>> +             list_for_each_entry(table, &afi->tables, list) {
>> +                     if (table->handle == handle)
>> +                             table_handle_check_flag = 1;
>
> Use:
>                                 return table;
>
> instead.

I have tried to do that but we need to have afi struct for flushing
the tables so nft_afinfo_lookup_byhandle is required iirc.
ctx.afi = afi;
ctx.table = table;

>
>> +             }
>> +             if (table_handle_check_flag)
>> +                     return afi;
>> +     }
>> +     return NULL;
>> +}
>> +
>>  static struct nft_af_info *
>>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>>  {
>> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>>       return ERR_PTR(-EAFNOSUPPORT);
>>  }
>>
>> +static struct nft_af_info *
>> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
>> +{
>> +     struct nft_af_info *afi;
>> +
>> +     afi = nft_afinfo_lookup_byhandle(net, handle);
>> +     if (afi != NULL)
>> +             return afi;
>> +#ifdef CONFIG_MODULES
>> +     if (autoload) {
>> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
>> +             request_module("nft-afinfo");
>> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
>> +             afi = nft_afinfo_lookup_byhandle(net, handle);
>> +             if (afi != NULL)
>> +                     return ERR_PTR(-EAGAIN);
>> +     }
>> +#endif
>> +     return ERR_PTR(-EAFNOSUPPORT);
>> +}
>
> I don't think you need this new nf_tables_afinfo_lookup_byhandle()
> function. The handle parameter is never used. That will simplify your
> patchset.

Using handle parameter in nft_afinfo_lookup_byhandle allows returning
afi for which afi->family is same as family of table (which has to be
deleted via table handle).
For deleting table via table name, family is required (unless default
ip family) nft delete table ip6 test-ip6, but as handle identifies
each table uniquely, a check is required
in nft_afinfo_lookup_byhandle for returning afi struct.
So, this new func nf_tables_afinfo_lookup_byhandle is required for
calling nft_afinfo_lookup_byhandle and otherwise returning error and
also for checking CONFIG_MODULES.
Thanks for the review. :)

>
>> +
>>  static void nft_ctx_init(struct nft_ctx *ctx,
>>                        struct net *net,
>>                        const struct sk_buff *skb,
>> @@ -107,7 +146,7 @@ static void nft_ctx_init(struct nft_ctx *ctx,
>>       ctx->afi        = afi;
>>       ctx->table      = table;
>>       ctx->chain      = chain;
>> -     ctx->nla        = nla;
>> +     ctx->nla        = nla;
>>       ctx->portid     = NETLINK_CB(skb).portid;
>>       ctx->report     = nlmsg_report(nlh);
>>       ctx->seq        = nlh->nlmsg_seq;
>> @@ -367,6 +406,28 @@ static struct nft_table *nft_table_lookup(const struct nft_af_info *afi,
>>       return NULL;
>>  }
>>
>> +static struct nft_table *__nft_table_lookup_byhandle(const struct nft_af_info *afi,
>> +                                                  u64 handle, u8 genmask)
>> +{
>> +     struct nft_table *table;
>> +
>> +     list_for_each_entry(table, &afi->tables, list) {
>> +             if (handle == table->handle &&
>> +                 nft_active_genmask(table, genmask))
>> +                     return table;
>> +     }
>> +     return NULL;
>> +}
>> +
>> +static struct nft_table *nft_table_lookup_byhandle(const struct nft_af_info *afi,
>> +                                                const struct nlattr *nla,
>> +                                                u8 genmask)
>> +{
>> +     return __nft_table_lookup_byhandle(afi,
>> +                                        be64_to_cpu(nla_get_be64(nla)),
>> +                                        genmask);
>> +}
>> +
>>  static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
>>                                               const struct nlattr *nla,
>>                                               u8 genmask)
>> @@ -383,6 +444,22 @@ static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
>>       return ERR_PTR(-ENOENT);
>>  }
>>
>> +static struct nft_table *nf_tables_table_lookup_byhandle(const struct nft_af_info *afi,
>> +                                                      const struct nlattr *nla,
>> +                                                      u8 genmask)
>> +{
>> +     struct nft_table *table;
>> +
>> +     if (nla == NULL)
>> +             return ERR_PTR(-EINVAL);
>> +
>> +     table = nft_table_lookup_byhandle(afi, nla, genmask);
>> +     if (table != NULL)
>> +             return table;
>> +
>> +     return ERR_PTR(-ENOENT);
>> +}
>> +
>>  static inline u64 nf_tables_alloc_handle(struct nft_table *table)
>>  {
>>       return ++table->hgenerator;
>> @@ -854,14 +931,22 @@ static int nf_tables_deltable(struct net *net, struct sock *nlsk,
>>       struct nft_ctx ctx;
>>
>>       nft_ctx_init(&ctx, net, skb, nlh, NULL, NULL, NULL, nla);
>> -     if (family == AF_UNSPEC || nla[NFTA_TABLE_NAME] == NULL)
>> +     if (nla[NFTA_TABLE_HANDLE]) {
>> +             afi = nf_tables_afinfo_lookup_byhandle(net, be64_to_cpu(nla_get_be64(nla[NFTA_TABLE_HANDLE])), false);
>> +             if (IS_ERR(afi))
>> +                     return PTR_ERR(afi);
>> +             else
>> +                     table = nf_tables_table_lookup_byhandle(afi, nla[NFTA_TABLE_HANDLE], genmask);
>> +     } else if (family == AF_UNSPEC || nla[NFTA_TABLE_NAME] == NULL) {
>>               return nft_flush(&ctx, family);
>> +     } else {
>>
>> -     afi = nf_tables_afinfo_lookup(net, family, false);
>> -     if (IS_ERR(afi))
>> -             return PTR_ERR(afi);
>> -
>> -     table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME], genmask);
>> +             afi = nf_tables_afinfo_lookup(net, family, false);
>> +             if (IS_ERR(afi))
>> +                     return PTR_ERR(afi);
>> +             else
>> +                     table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME], genmask);
>> +     }
>>       if (IS_ERR(table))
>>               return PTR_ERR(table);
>>
>> --
>> 2.11.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Jan. 7, 2018, 5:56 p.m. UTC | #3
On Sun, Jan 07, 2018 at 02:49:29PM +0530, Harsha Sharma wrote:
> On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
> >> This patch add code to delete table via unique table handle.
> >>
> >> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
> >> ---
> >>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
> >>  1 file changed, 92 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> >> index dabdd2ed66c8..3b1c879fdf61 100644
> >> --- a/net/netfilter/nf_tables_api.c
> >> +++ b/net/netfilter/nf_tables_api.c
> >> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
> >>       return NULL;
> >>  }
> >>
> >> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
> >> +                                                   u64 handle)
> >> +{
> >> +     struct nft_af_info *afi;
> >> +     struct nft_table *table;
> >> +     int table_handle_check_flag = 0;
> >> +
> >> +     list_for_each_entry(afi, &net->nft.af_info, list) {
> >> +             list_for_each_entry(table, &afi->tables, list) {
> >> +                     if (table->handle == handle)
> >> +                             table_handle_check_flag = 1;
> >
> > Use:
> >                                 return table;
> >
> > instead.
> 
> I have tried to do that but we need to have afi struct for flushing
> the tables so nft_afinfo_lookup_byhandle is required iirc.
> ctx.afi = afi;
> ctx.table = table;

If you need the afi structure, you can get the afi via the existing
nft_afinfo_lookup() function.

> >> +             }
> >> +             if (table_handle_check_flag)
> >> +                     return afi;
> >> +     }
> >> +     return NULL;
> >> +}
> >> +
> >>  static struct nft_af_info *
> >>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
> >>  {
> >> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
> >>       return ERR_PTR(-EAFNOSUPPORT);
> >>  }
> >>
> >> +static struct nft_af_info *
> >> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
> >> +{
> >> +     struct nft_af_info *afi;
> >> +
> >> +     afi = nft_afinfo_lookup_byhandle(net, handle);
> >> +     if (afi != NULL)
> >> +             return afi;
> >> +#ifdef CONFIG_MODULES
> >> +     if (autoload) {
> >> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
> >> +             request_module("nft-afinfo");
> >> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
> >> +             afi = nft_afinfo_lookup_byhandle(net, handle);
> >> +             if (afi != NULL)
> >> +                     return ERR_PTR(-EAGAIN);
> >> +     }
> >> +#endif
> >> +     return ERR_PTR(-EAFNOSUPPORT);
> >> +}
> >
> > I don't think you need this new nf_tables_afinfo_lookup_byhandle()
> > function. The handle parameter is never used. That will simplify your
> > patchset.
> 
> Using handle parameter in nft_afinfo_lookup_byhandle allows returning
> afi for which afi->family is same as family of table (which has to be
> deleted via table handle).
> For deleting table via table name, family is required (unless default
> ip family) nft delete table ip6 test-ip6, but as handle identifies
> each table uniquely, a check is required
> in nft_afinfo_lookup_byhandle for returning afi struct.
> So, this new func nf_tables_afinfo_lookup_byhandle is required for
> calling nft_afinfo_lookup_byhandle and otherwise returning error and
> also for checking CONFIG_MODULES.

Nope :), this is not required. You can just:

#1 Get afi structure via existing nft_afinfo_lookup() function.
#2 Call your new nf_tables_table_lookup_byhandle() function.

That will simplify this patch.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Harsha Sharma Jan. 7, 2018, 6:10 p.m. UTC | #4
On Sun, Jan 7, 2018 at 11:26 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jan 07, 2018 at 02:49:29PM +0530, Harsha Sharma wrote:
>> On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
>> >> This patch add code to delete table via unique table handle.
>> >>
>> >> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
>> >> ---
>> >>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
>> >>  1 file changed, 92 insertions(+), 7 deletions(-)
>> >>
>> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> >> index dabdd2ed66c8..3b1c879fdf61 100644
>> >> --- a/net/netfilter/nf_tables_api.c
>> >> +++ b/net/netfilter/nf_tables_api.c
>> >> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
>> >>       return NULL;
>> >>  }
>> >>
>> >> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
>> >> +                                                   u64 handle)
>> >> +{
>> >> +     struct nft_af_info *afi;
>> >> +     struct nft_table *table;
>> >> +     int table_handle_check_flag = 0;
>> >> +
>> >> +     list_for_each_entry(afi, &net->nft.af_info, list) {
>> >> +             list_for_each_entry(table, &afi->tables, list) {
>> >> +                     if (table->handle == handle)
>> >> +                             table_handle_check_flag = 1;
>> >
>> > Use:
>> >                                 return table;
>> >
>> > instead.
>>
>> I have tried to do that but we need to have afi struct for flushing
>> the tables so nft_afinfo_lookup_byhandle is required iirc.
>> ctx.afi = afi;
>> ctx.table = table;
>
> If you need the afi structure, you can get the afi via the existing
> nft_afinfo_lookup() function.
>
>> >> +             }
>> >> +             if (table_handle_check_flag)
>> >> +                     return afi;
>> >> +     }
>> >> +     return NULL;
>> >> +}
>> >> +
>> >>  static struct nft_af_info *
>> >>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>> >>  {
>> >> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>> >>       return ERR_PTR(-EAFNOSUPPORT);
>> >>  }
>> >>
>> >> +static struct nft_af_info *
>> >> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
>> >> +{
>> >> +     struct nft_af_info *afi;
>> >> +
>> >> +     afi = nft_afinfo_lookup_byhandle(net, handle);
>> >> +     if (afi != NULL)
>> >> +             return afi;
>> >> +#ifdef CONFIG_MODULES
>> >> +     if (autoload) {
>> >> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
>> >> +             request_module("nft-afinfo");
>> >> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
>> >> +             afi = nft_afinfo_lookup_byhandle(net, handle);
>> >> +             if (afi != NULL)
>> >> +                     return ERR_PTR(-EAGAIN);
>> >> +     }
>> >> +#endif
>> >> +     return ERR_PTR(-EAFNOSUPPORT);
>> >> +}
>> >
>> > I don't think you need this new nf_tables_afinfo_lookup_byhandle()
>> > function. The handle parameter is never used. That will simplify your
>> > patchset.
>>
>> Using handle parameter in nft_afinfo_lookup_byhandle allows returning
>> afi for which afi->family is same as family of table (which has to be
>> deleted via table handle).
>> For deleting table via table name, family is required (unless default
>> ip family) nft delete table ip6 test-ip6, but as handle identifies
>> each table uniquely, a check is required
>> in nft_afinfo_lookup_byhandle for returning afi struct.
>> So, this new func nf_tables_afinfo_lookup_byhandle is required for
>> calling nft_afinfo_lookup_byhandle and otherwise returning error and
>> also for checking CONFIG_MODULES.
>
> Nope :), this is not required. You can just:
>
> #1 Get afi structure via existing nft_afinfo_lookup() function.

I have tried that but with that I'm not able to delete table families
other than ip.
With (e.g nft delete table handle 4 ), as no family is scpecified (it
doesn't even make sense to specify family with handle), family is
defaulted to 2 (for ip) and nft_afiinfo_lookup return afi with family
2, and this function allows returning afi with family same as family
of table which has to be deleted via table handle.

> #2 Call your new nf_tables_table_lookup_byhandle() function.
>
> That will simplify this patch.

I hope that makes sense.
Thanks for your time and Can I get some more tasks ?

Regards,
Harsha Sharma
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Jan. 7, 2018, 6:16 p.m. UTC | #5
On Sun, Jan 07, 2018 at 11:40:47PM +0530, Harsha Sharma wrote:
> On Sun, Jan 7, 2018 at 11:26 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Jan 07, 2018 at 02:49:29PM +0530, Harsha Sharma wrote:
> >> On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
> >> >> This patch add code to delete table via unique table handle.
> >> >>
> >> >> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
> >> >> ---
> >> >>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
> >> >>  1 file changed, 92 insertions(+), 7 deletions(-)
> >> >>
> >> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> >> >> index dabdd2ed66c8..3b1c879fdf61 100644
> >> >> --- a/net/netfilter/nf_tables_api.c
> >> >> +++ b/net/netfilter/nf_tables_api.c
> >> >> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
> >> >>       return NULL;
> >> >>  }
> >> >>
> >> >> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
> >> >> +                                                   u64 handle)
> >> >> +{
> >> >> +     struct nft_af_info *afi;
> >> >> +     struct nft_table *table;
> >> >> +     int table_handle_check_flag = 0;
> >> >> +
> >> >> +     list_for_each_entry(afi, &net->nft.af_info, list) {
> >> >> +             list_for_each_entry(table, &afi->tables, list) {
> >> >> +                     if (table->handle == handle)
> >> >> +                             table_handle_check_flag = 1;
> >> >
> >> > Use:
> >> >                                 return table;
> >> >
> >> > instead.
> >>
> >> I have tried to do that but we need to have afi struct for flushing
> >> the tables so nft_afinfo_lookup_byhandle is required iirc.
> >> ctx.afi = afi;
> >> ctx.table = table;
> >
> > If you need the afi structure, you can get the afi via the existing
> > nft_afinfo_lookup() function.
> >
> >> >> +             }
> >> >> +             if (table_handle_check_flag)
> >> >> +                     return afi;
> >> >> +     }
> >> >> +     return NULL;
> >> >> +}
> >> >> +
> >> >>  static struct nft_af_info *
> >> >>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
> >> >>  {
> >> >> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
> >> >>       return ERR_PTR(-EAFNOSUPPORT);
> >> >>  }
> >> >>
> >> >> +static struct nft_af_info *
> >> >> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
> >> >> +{
> >> >> +     struct nft_af_info *afi;
> >> >> +
> >> >> +     afi = nft_afinfo_lookup_byhandle(net, handle);
> >> >> +     if (afi != NULL)
> >> >> +             return afi;
> >> >> +#ifdef CONFIG_MODULES
> >> >> +     if (autoload) {
> >> >> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
> >> >> +             request_module("nft-afinfo");
> >> >> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
> >> >> +             afi = nft_afinfo_lookup_byhandle(net, handle);
> >> >> +             if (afi != NULL)
> >> >> +                     return ERR_PTR(-EAGAIN);
> >> >> +     }
> >> >> +#endif
> >> >> +     return ERR_PTR(-EAFNOSUPPORT);
> >> >> +}
> >> >
> >> > I don't think you need this new nf_tables_afinfo_lookup_byhandle()
> >> > function. The handle parameter is never used. That will simplify your
> >> > patchset.
> >>
> >> Using handle parameter in nft_afinfo_lookup_byhandle allows returning
> >> afi for which afi->family is same as family of table (which has to be
> >> deleted via table handle).
> >> For deleting table via table name, family is required (unless default
> >> ip family) nft delete table ip6 test-ip6, but as handle identifies
> >> each table uniquely, a check is required
> >> in nft_afinfo_lookup_byhandle for returning afi struct.
> >> So, this new func nf_tables_afinfo_lookup_byhandle is required for
> >> calling nft_afinfo_lookup_byhandle and otherwise returning error and
> >> also for checking CONFIG_MODULES.
> >
> > Nope :), this is not required. You can just:
> >
> > #1 Get afi structure via existing nft_afinfo_lookup() function.
> 
> I have tried that but with that I'm not able to delete table families
> other than ip.
> With (e.g nft delete table handle 4 ), as no family is scpecified (it
> doesn't even make sense to specify family with handle), family is
> defaulted to 2 (for ip) and nft_afiinfo_lookup return afi with family
> 2, and this function allows returning afi with family same as family
> of table which has to be deleted via table handle.

If you want to delete an ip6 table, you have to specify this from the
command line, ie.

        nft delete table ip6 handle 4
                         ^^^

Then, family because NFPROTO_IPV6, which is what we pass to:

        afi = nf_tables_afinfo_lookup(net, family, false);

as the second parameter for this function.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Harsha Sharma Jan. 7, 2018, 6:28 p.m. UTC | #6
On Sun, Jan 7, 2018 at 11:46 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jan 07, 2018 at 11:40:47PM +0530, Harsha Sharma wrote:
>> On Sun, Jan 7, 2018 at 11:26 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Sun, Jan 07, 2018 at 02:49:29PM +0530, Harsha Sharma wrote:
>> >> On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >> > On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
>> >> >> This patch add code to delete table via unique table handle.
>> >> >>
>> >> >> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
>> >> >> ---
>> >> >>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
>> >> >>  1 file changed, 92 insertions(+), 7 deletions(-)
>> >> >>
>> >> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> >> >> index dabdd2ed66c8..3b1c879fdf61 100644
>> >> >> --- a/net/netfilter/nf_tables_api.c
>> >> >> +++ b/net/netfilter/nf_tables_api.c
>> >> >> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
>> >> >>       return NULL;
>> >> >>  }
>> >> >>
>> >> >> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
>> >> >> +                                                   u64 handle)
>> >> >> +{
>> >> >> +     struct nft_af_info *afi;
>> >> >> +     struct nft_table *table;
>> >> >> +     int table_handle_check_flag = 0;
>> >> >> +
>> >> >> +     list_for_each_entry(afi, &net->nft.af_info, list) {
>> >> >> +             list_for_each_entry(table, &afi->tables, list) {
>> >> >> +                     if (table->handle == handle)
>> >> >> +                             table_handle_check_flag = 1;
>> >> >
>> >> > Use:
>> >> >                                 return table;
>> >> >
>> >> > instead.
>> >>
>> >> I have tried to do that but we need to have afi struct for flushing
>> >> the tables so nft_afinfo_lookup_byhandle is required iirc.
>> >> ctx.afi = afi;
>> >> ctx.table = table;
>> >
>> > If you need the afi structure, you can get the afi via the existing
>> > nft_afinfo_lookup() function.
>> >
>> >> >> +             }
>> >> >> +             if (table_handle_check_flag)
>> >> >> +                     return afi;
>> >> >> +     }
>> >> >> +     return NULL;
>> >> >> +}
>> >> >> +
>> >> >>  static struct nft_af_info *
>> >> >>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>> >> >>  {
>> >> >> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>> >> >>       return ERR_PTR(-EAFNOSUPPORT);
>> >> >>  }
>> >> >>
>> >> >> +static struct nft_af_info *
>> >> >> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
>> >> >> +{
>> >> >> +     struct nft_af_info *afi;
>> >> >> +
>> >> >> +     afi = nft_afinfo_lookup_byhandle(net, handle);
>> >> >> +     if (afi != NULL)
>> >> >> +             return afi;
>> >> >> +#ifdef CONFIG_MODULES
>> >> >> +     if (autoload) {
>> >> >> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
>> >> >> +             request_module("nft-afinfo");
>> >> >> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
>> >> >> +             afi = nft_afinfo_lookup_byhandle(net, handle);
>> >> >> +             if (afi != NULL)
>> >> >> +                     return ERR_PTR(-EAGAIN);
>> >> >> +     }
>> >> >> +#endif
>> >> >> +     return ERR_PTR(-EAFNOSUPPORT);
>> >> >> +}
>> >> >
>> >> > I don't think you need this new nf_tables_afinfo_lookup_byhandle()
>> >> > function. The handle parameter is never used. That will simplify your
>> >> > patchset.
>> >>
>> >> Using handle parameter in nft_afinfo_lookup_byhandle allows returning
>> >> afi for which afi->family is same as family of table (which has to be
>> >> deleted via table handle).
>> >> For deleting table via table name, family is required (unless default
>> >> ip family) nft delete table ip6 test-ip6, but as handle identifies
>> >> each table uniquely, a check is required
>> >> in nft_afinfo_lookup_byhandle for returning afi struct.
>> >> So, this new func nf_tables_afinfo_lookup_byhandle is required for
>> >> calling nft_afinfo_lookup_byhandle and otherwise returning error and
>> >> also for checking CONFIG_MODULES.
>> >
>> > Nope :), this is not required. You can just:
>> >
>> > #1 Get afi structure via existing nft_afinfo_lookup() function.
>>
>> I have tried that but with that I'm not able to delete table families
>> other than ip.
>> With (e.g nft delete table handle 4 ), as no family is scpecified (it
>> doesn't even make sense to specify family with handle), family is
>> defaulted to 2 (for ip) and nft_afiinfo_lookup return afi with family
>> 2, and this function allows returning afi with family same as family
>> of table which has to be deleted via table handle.
>
> If you want to delete an ip6 table, you have to specify this from the
> command line, ie.
>
>         nft delete table ip6 handle 4
>                          ^^^

But if the handle uniquely identifies table, then we should be able to
delete the table via table handle only.
As there can be two tables with same name but different family, we
should specify family for deleting table via table name but as there
is no same handle for any two tables, we should we able to delete
table via table handle only.

> Then, family because NFPROTO_IPV6, which is what we pass to:
>
>         afi = nf_tables_afinfo_lookup(net, family, false);
>
> as the second parameter for this function.

Let me know which option seems better.
Thanks for your time :)

Regards,
Harsha Sharma
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Jan. 7, 2018, 6:51 p.m. UTC | #7
On Sun, Jan 07, 2018 at 11:58:49PM +0530, Harsha Sharma wrote:
> On Sun, Jan 7, 2018 at 11:46 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Sun, Jan 07, 2018 at 11:40:47PM +0530, Harsha Sharma wrote:
> >> On Sun, Jan 7, 2018 at 11:26 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> > On Sun, Jan 07, 2018 at 02:49:29PM +0530, Harsha Sharma wrote:
> >> >> On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >> >> > On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
> >> >> >> This patch add code to delete table via unique table handle.
> >> >> >>
> >> >> >> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
> >> >> >> ---
> >> >> >>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
> >> >> >>  1 file changed, 92 insertions(+), 7 deletions(-)
> >> >> >>
> >> >> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
> >> >> >> index dabdd2ed66c8..3b1c879fdf61 100644
> >> >> >> --- a/net/netfilter/nf_tables_api.c
> >> >> >> +++ b/net/netfilter/nf_tables_api.c
> >> >> >> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
> >> >> >>       return NULL;
> >> >> >>  }
> >> >> >>
> >> >> >> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
> >> >> >> +                                                   u64 handle)
> >> >> >> +{
> >> >> >> +     struct nft_af_info *afi;
> >> >> >> +     struct nft_table *table;
> >> >> >> +     int table_handle_check_flag = 0;
> >> >> >> +
> >> >> >> +     list_for_each_entry(afi, &net->nft.af_info, list) {
> >> >> >> +             list_for_each_entry(table, &afi->tables, list) {
> >> >> >> +                     if (table->handle == handle)
> >> >> >> +                             table_handle_check_flag = 1;
> >> >> >
> >> >> > Use:
> >> >> >                                 return table;
> >> >> >
> >> >> > instead.
> >> >>
> >> >> I have tried to do that but we need to have afi struct for flushing
> >> >> the tables so nft_afinfo_lookup_byhandle is required iirc.
> >> >> ctx.afi = afi;
> >> >> ctx.table = table;
> >> >
> >> > If you need the afi structure, you can get the afi via the existing
> >> > nft_afinfo_lookup() function.
> >> >
> >> >> >> +             }
> >> >> >> +             if (table_handle_check_flag)
> >> >> >> +                     return afi;
> >> >> >> +     }
> >> >> >> +     return NULL;
> >> >> >> +}
> >> >> >> +
> >> >> >>  static struct nft_af_info *
> >> >> >>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
> >> >> >>  {
> >> >> >> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
> >> >> >>       return ERR_PTR(-EAFNOSUPPORT);
> >> >> >>  }
> >> >> >>
> >> >> >> +static struct nft_af_info *
> >> >> >> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
> >> >> >> +{
> >> >> >> +     struct nft_af_info *afi;
> >> >> >> +
> >> >> >> +     afi = nft_afinfo_lookup_byhandle(net, handle);
> >> >> >> +     if (afi != NULL)
> >> >> >> +             return afi;
> >> >> >> +#ifdef CONFIG_MODULES
> >> >> >> +     if (autoload) {
> >> >> >> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
> >> >> >> +             request_module("nft-afinfo");
> >> >> >> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
> >> >> >> +             afi = nft_afinfo_lookup_byhandle(net, handle);
> >> >> >> +             if (afi != NULL)
> >> >> >> +                     return ERR_PTR(-EAGAIN);
> >> >> >> +     }
> >> >> >> +#endif
> >> >> >> +     return ERR_PTR(-EAFNOSUPPORT);
> >> >> >> +}
> >> >> >
> >> >> > I don't think you need this new nf_tables_afinfo_lookup_byhandle()
> >> >> > function. The handle parameter is never used. That will simplify your
> >> >> > patchset.
> >> >>
> >> >> Using handle parameter in nft_afinfo_lookup_byhandle allows returning
> >> >> afi for which afi->family is same as family of table (which has to be
> >> >> deleted via table handle).
> >> >> For deleting table via table name, family is required (unless default
> >> >> ip family) nft delete table ip6 test-ip6, but as handle identifies
> >> >> each table uniquely, a check is required
> >> >> in nft_afinfo_lookup_byhandle for returning afi struct.
> >> >> So, this new func nf_tables_afinfo_lookup_byhandle is required for
> >> >> calling nft_afinfo_lookup_byhandle and otherwise returning error and
> >> >> also for checking CONFIG_MODULES.
> >> >
> >> > Nope :), this is not required. You can just:
> >> >
> >> > #1 Get afi structure via existing nft_afinfo_lookup() function.
> >>
> >> I have tried that but with that I'm not able to delete table families
> >> other than ip.
> >> With (e.g nft delete table handle 4 ), as no family is scpecified (it
> >> doesn't even make sense to specify family with handle), family is
> >> defaulted to 2 (for ip) and nft_afiinfo_lookup return afi with family
> >> 2, and this function allows returning afi with family same as family
> >> of table which has to be deleted via table handle.
> >
> > If you want to delete an ip6 table, you have to specify this from the
> > command line, ie.
> >
> >         nft delete table ip6 handle 4
> >                          ^^^
> 
> But if the handle uniquely identifies table, then we should be able to
> delete the table via table handle only.

I see your point in that the handle number is already unique.

However, we need to provide consistent behaviour with regards to what
we already support via rule deletion:

        nft delete rule ip handle 3
        nft delete rule ip6 handle 4

And there, family is always specified.

Note that:

        nft delete rule handle 3

actually _implicitly_ means 'ip', so if we add this feature in the way
you propose, we'll introduce an inconsistency given "no family
specified" means "default to ip".

Anyway, we can revisit this in the future, to allow to delete things
only by handle, but this would be new feature. Let's keep this in mind
for the future.

At this stage, what I'm suggesting will simplify your patchset.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Harsha Sharma Jan. 7, 2018, 7:08 p.m. UTC | #8
On Mon, Jan 8, 2018 at 12:21 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Sun, Jan 07, 2018 at 11:58:49PM +0530, Harsha Sharma wrote:
>> On Sun, Jan 7, 2018 at 11:46 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> > On Sun, Jan 07, 2018 at 11:40:47PM +0530, Harsha Sharma wrote:
>> >> On Sun, Jan 7, 2018 at 11:26 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >> > On Sun, Jan 07, 2018 at 02:49:29PM +0530, Harsha Sharma wrote:
>> >> >> On Sun, Jan 7, 2018 at 4:32 AM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>> >> >> > On Sun, Jan 07, 2018 at 12:00:15AM +0530, Harsha Sharma wrote:
>> >> >> >> This patch add code to delete table via unique table handle.
>> >> >> >>
>> >> >> >> Signed-off-by: Harsha Sharma <harshasharmaiitr@gmail.com>
>> >> >> >> ---
>> >> >> >>  net/netfilter/nf_tables_api.c | 99 ++++++++++++++++++++++++++++++++++++++++---
>> >> >> >>  1 file changed, 92 insertions(+), 7 deletions(-)
>> >> >> >>
>> >> >> >> diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
>> >> >> >> index dabdd2ed66c8..3b1c879fdf61 100644
>> >> >> >> --- a/net/netfilter/nf_tables_api.c
>> >> >> >> +++ b/net/netfilter/nf_tables_api.c
>> >> >> >> @@ -73,6 +73,24 @@ static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
>> >> >> >>       return NULL;
>> >> >> >>  }
>> >> >> >>
>> >> >> >> +static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
>> >> >> >> +                                                   u64 handle)
>> >> >> >> +{
>> >> >> >> +     struct nft_af_info *afi;
>> >> >> >> +     struct nft_table *table;
>> >> >> >> +     int table_handle_check_flag = 0;
>> >> >> >> +
>> >> >> >> +     list_for_each_entry(afi, &net->nft.af_info, list) {
>> >> >> >> +             list_for_each_entry(table, &afi->tables, list) {
>> >> >> >> +                     if (table->handle == handle)
>> >> >> >> +                             table_handle_check_flag = 1;
>> >> >> >
>> >> >> > Use:
>> >> >> >                                 return table;
>> >> >> >
>> >> >> > instead.
>> >> >>
>> >> >> I have tried to do that but we need to have afi struct for flushing
>> >> >> the tables so nft_afinfo_lookup_byhandle is required iirc.
>> >> >> ctx.afi = afi;
>> >> >> ctx.table = table;
>> >> >
>> >> > If you need the afi structure, you can get the afi via the existing
>> >> > nft_afinfo_lookup() function.
>> >> >
>> >> >> >> +             }
>> >> >> >> +             if (table_handle_check_flag)
>> >> >> >> +                     return afi;
>> >> >> >> +     }
>> >> >> >> +     return NULL;
>> >> >> >> +}
>> >> >> >> +
>> >> >> >>  static struct nft_af_info *
>> >> >> >>  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>> >> >> >>  {
>> >> >> >> @@ -94,6 +112,27 @@ nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
>> >> >> >>       return ERR_PTR(-EAFNOSUPPORT);
>> >> >> >>  }
>> >> >> >>
>> >> >> >> +static struct nft_af_info *
>> >> >> >> +nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
>> >> >> >> +{
>> >> >> >> +     struct nft_af_info *afi;
>> >> >> >> +
>> >> >> >> +     afi = nft_afinfo_lookup_byhandle(net, handle);
>> >> >> >> +     if (afi != NULL)
>> >> >> >> +             return afi;
>> >> >> >> +#ifdef CONFIG_MODULES
>> >> >> >> +     if (autoload) {
>> >> >> >> +             nfnl_unlock(NFNL_SUBSYS_NFTABLES);
>> >> >> >> +             request_module("nft-afinfo");
>> >> >> >> +             nfnl_lock(NFNL_SUBSYS_NFTABLES);
>> >> >> >> +             afi = nft_afinfo_lookup_byhandle(net, handle);
>> >> >> >> +             if (afi != NULL)
>> >> >> >> +                     return ERR_PTR(-EAGAIN);
>> >> >> >> +     }
>> >> >> >> +#endif
>> >> >> >> +     return ERR_PTR(-EAFNOSUPPORT);
>> >> >> >> +}
>> >> >> >
>> >> >> > I don't think you need this new nf_tables_afinfo_lookup_byhandle()
>> >> >> > function. The handle parameter is never used. That will simplify your
>> >> >> > patchset.
>> >> >>
>> >> >> Using handle parameter in nft_afinfo_lookup_byhandle allows returning
>> >> >> afi for which afi->family is same as family of table (which has to be
>> >> >> deleted via table handle).
>> >> >> For deleting table via table name, family is required (unless default
>> >> >> ip family) nft delete table ip6 test-ip6, but as handle identifies
>> >> >> each table uniquely, a check is required
>> >> >> in nft_afinfo_lookup_byhandle for returning afi struct.
>> >> >> So, this new func nf_tables_afinfo_lookup_byhandle is required for
>> >> >> calling nft_afinfo_lookup_byhandle and otherwise returning error and
>> >> >> also for checking CONFIG_MODULES.
>> >> >
>> >> > Nope :), this is not required. You can just:
>> >> >
>> >> > #1 Get afi structure via existing nft_afinfo_lookup() function.
>> >>
>> >> I have tried that but with that I'm not able to delete table families
>> >> other than ip.
>> >> With (e.g nft delete table handle 4 ), as no family is scpecified (it
>> >> doesn't even make sense to specify family with handle), family is
>> >> defaulted to 2 (for ip) and nft_afiinfo_lookup return afi with family
>> >> 2, and this function allows returning afi with family same as family
>> >> of table which has to be deleted via table handle.
>> >
>> > If you want to delete an ip6 table, you have to specify this from the
>> > command line, ie.
>> >
>> >         nft delete table ip6 handle 4
>> >                          ^^^
>>
>> But if the handle uniquely identifies table, then we should be able to
>> delete the table via table handle only.
>
> I see your point in that the handle number is already unique.
>
> However, we need to provide consistent behaviour with regards to what
> we already support via rule deletion:
>
>         nft delete rule ip handle 3
>         nft delete rule ip6 handle 4
>
> And there, family is always specified.
>
> Note that:
>
>         nft delete rule handle 3
>
> actually _implicitly_ means 'ip', so if we add this feature in the way
> you propose, we'll introduce an inconsistency given "no family
> specified" means "default to ip".
>
> Anyway, we can revisit this in the future, to allow to delete things
> only by handle, but this would be new feature. Let's keep this in mind
> for the future.
>
> At this stage, what I'm suggesting will simplify your patchset.

Yes,okay. I'll send a v2 for this.
Thanks for the quick review. :)

> Thanks!
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox series

Patch

diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c
index dabdd2ed66c8..3b1c879fdf61 100644
--- a/net/netfilter/nf_tables_api.c
+++ b/net/netfilter/nf_tables_api.c
@@ -73,6 +73,24 @@  static struct nft_af_info *nft_afinfo_lookup(struct net *net, int family)
 	return NULL;
 }
 
+static struct nft_af_info *nft_afinfo_lookup_byhandle(struct net *net,
+						      u64 handle)
+{
+	struct nft_af_info *afi;
+	struct nft_table *table;
+	int table_handle_check_flag = 0;
+
+	list_for_each_entry(afi, &net->nft.af_info, list) {
+		list_for_each_entry(table, &afi->tables, list) {
+			if (table->handle == handle)
+				table_handle_check_flag = 1;
+		}
+		if (table_handle_check_flag)
+			return afi;
+	}
+	return NULL;
+}
+
 static struct nft_af_info *
 nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
 {
@@ -94,6 +112,27 @@  nf_tables_afinfo_lookup(struct net *net, int family, bool autoload)
 	return ERR_PTR(-EAFNOSUPPORT);
 }
 
+static struct nft_af_info *
+nf_tables_afinfo_lookup_byhandle(struct net *net, u64 handle, bool autoload)
+{
+	struct nft_af_info *afi;
+
+	afi = nft_afinfo_lookup_byhandle(net, handle);
+	if (afi != NULL)
+		return afi;
+#ifdef CONFIG_MODULES
+	if (autoload) {
+		nfnl_unlock(NFNL_SUBSYS_NFTABLES);
+		request_module("nft-afinfo");
+		nfnl_lock(NFNL_SUBSYS_NFTABLES);
+		afi = nft_afinfo_lookup_byhandle(net, handle);
+		if (afi != NULL)
+			return ERR_PTR(-EAGAIN);
+	}
+#endif
+	return ERR_PTR(-EAFNOSUPPORT);
+}
+
 static void nft_ctx_init(struct nft_ctx *ctx,
 			 struct net *net,
 			 const struct sk_buff *skb,
@@ -107,7 +146,7 @@  static void nft_ctx_init(struct nft_ctx *ctx,
 	ctx->afi	= afi;
 	ctx->table	= table;
 	ctx->chain	= chain;
-	ctx->nla   	= nla;
+	ctx->nla	= nla;
 	ctx->portid	= NETLINK_CB(skb).portid;
 	ctx->report	= nlmsg_report(nlh);
 	ctx->seq	= nlh->nlmsg_seq;
@@ -367,6 +406,28 @@  static struct nft_table *nft_table_lookup(const struct nft_af_info *afi,
 	return NULL;
 }
 
+static struct nft_table *__nft_table_lookup_byhandle(const struct nft_af_info *afi,
+						     u64 handle, u8 genmask)
+{
+	struct nft_table *table;
+
+	list_for_each_entry(table, &afi->tables, list) {
+		if (handle == table->handle &&
+		    nft_active_genmask(table, genmask))
+			return table;
+	}
+	return NULL;
+}
+
+static struct nft_table *nft_table_lookup_byhandle(const struct nft_af_info *afi,
+						   const struct nlattr *nla,
+						   u8 genmask)
+{
+	return __nft_table_lookup_byhandle(afi,
+					   be64_to_cpu(nla_get_be64(nla)),
+					   genmask);
+}
+
 static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
 						const struct nlattr *nla,
 						u8 genmask)
@@ -383,6 +444,22 @@  static struct nft_table *nf_tables_table_lookup(const struct nft_af_info *afi,
 	return ERR_PTR(-ENOENT);
 }
 
+static struct nft_table *nf_tables_table_lookup_byhandle(const struct nft_af_info *afi,
+							 const struct nlattr *nla,
+							 u8 genmask)
+{
+	struct nft_table *table;
+
+	if (nla == NULL)
+		return ERR_PTR(-EINVAL);
+
+	table = nft_table_lookup_byhandle(afi, nla, genmask);
+	if (table != NULL)
+		return table;
+
+	return ERR_PTR(-ENOENT);
+}
+
 static inline u64 nf_tables_alloc_handle(struct nft_table *table)
 {
 	return ++table->hgenerator;
@@ -854,14 +931,22 @@  static int nf_tables_deltable(struct net *net, struct sock *nlsk,
 	struct nft_ctx ctx;
 
 	nft_ctx_init(&ctx, net, skb, nlh, NULL, NULL, NULL, nla);
-	if (family == AF_UNSPEC || nla[NFTA_TABLE_NAME] == NULL)
+	if (nla[NFTA_TABLE_HANDLE]) {
+		afi = nf_tables_afinfo_lookup_byhandle(net, be64_to_cpu(nla_get_be64(nla[NFTA_TABLE_HANDLE])), false);
+		if (IS_ERR(afi))
+			return PTR_ERR(afi);
+		else
+			table = nf_tables_table_lookup_byhandle(afi, nla[NFTA_TABLE_HANDLE], genmask);
+	} else if (family == AF_UNSPEC || nla[NFTA_TABLE_NAME] == NULL) {
 		return nft_flush(&ctx, family);
+	} else {
 
-	afi = nf_tables_afinfo_lookup(net, family, false);
-	if (IS_ERR(afi))
-		return PTR_ERR(afi);
-
-	table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME], genmask);
+		afi = nf_tables_afinfo_lookup(net, family, false);
+		if (IS_ERR(afi))
+			return PTR_ERR(afi);
+		else
+			table = nf_tables_table_lookup(afi, nla[NFTA_TABLE_NAME], genmask);
+	}
 	if (IS_ERR(table))
 		return PTR_ERR(table);