LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* net: portid signedness and format string fixes
@ 2015-03-13 11:31 Richard Weinberger
  2015-03-13 11:31 ` [PATCH 1/4] netlink: Fix portid type in netlink_notify Richard Weinberger
                   ` (3 more replies)
  0 siblings, 4 replies; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 11:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-wireless, coreteam, netfilter-devel, linux-kernel, sameo,
	aloisio.almeida, lauro.venancio, davem, kadlec, kaber, pablo

My application blow up as values in nfnetlink_queue suddenly turned
negative.

i.e.
$ cat /proc/net/netfilter/nfnetlink_queue
    0  29508   278 2 65531     0 2004213241 -2129885586  1
    1 -27747     0 2 65531     0     0        0  1
    2 -27748     0 2 65531     0     0        0  1

This series fixes that. Patches 1 to 3 change users of signed
portid to unsigned integers. Just for the sake of consistency.
Patch 4 fixes the printf format string such that applications
don't have to deal with negative numbers.

[PATCH 1/4] netlink: Fix portid type in netlink_notify
[PATCH 2/4] nfc: Fix portid type in urelease_work
[PATCH 3/4] netfilter: Fix portid types
[PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

Thanks,
//richard

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] netlink: Fix portid type in netlink_notify
  2015-03-13 11:31 net: portid signedness and format string fixes Richard Weinberger
@ 2015-03-13 11:31 ` Richard Weinberger
  2015-03-13 11:31 ` [PATCH 2/4] nfc: Fix portid type in urelease_work Richard Weinberger
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 11:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-wireless, coreteam, netfilter-devel, linux-kernel, sameo,
	aloisio.almeida, lauro.venancio, davem, kadlec, kaber, pablo,
	Richard Weinberger

portid is an unsigned integer. Fix netlink_notify to
match all other portid user in the kernel.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 include/linux/netlink.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/netlink.h b/include/linux/netlink.h
index 02fc86d..6a7e43e 100644
--- a/include/linux/netlink.h
+++ b/include/linux/netlink.h
@@ -134,7 +134,7 @@ struct netlink_callback {
 
 struct netlink_notify {
 	struct net *net;
-	int portid;
+	unsigned int portid;
 	int protocol;
 };
 
-- 
1.8.4.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/4] nfc: Fix portid type in urelease_work
  2015-03-13 11:31 net: portid signedness and format string fixes Richard Weinberger
  2015-03-13 11:31 ` [PATCH 1/4] netlink: Fix portid type in netlink_notify Richard Weinberger
@ 2015-03-13 11:31 ` Richard Weinberger
  2015-03-13 11:31 ` [PATCH 3/4] netfilter: Fix portid types Richard Weinberger
  2015-03-13 11:31 ` [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file Richard Weinberger
  3 siblings, 0 replies; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 11:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-wireless, coreteam, netfilter-devel, linux-kernel, sameo,
	aloisio.almeida, lauro.venancio, davem, kadlec, kaber, pablo,
	Richard Weinberger

portid is an unsigned integer. Fix urelease_work to
match all other portid user in the kernel.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 net/nfc/netlink.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/nfc/netlink.c b/net/nfc/netlink.c
index 14a2d11..1f12c4c 100644
--- a/net/nfc/netlink.c
+++ b/net/nfc/netlink.c
@@ -1583,8 +1583,8 @@ static const struct genl_ops nfc_genl_ops[] = {
 
 
 struct urelease_work {
-	struct	work_struct w;
-	int	portid;
+	struct		work_struct w;
+	unsigned int	portid;
 };
 
 static void nfc_urelease_event_work(struct work_struct *work)
-- 
1.8.4.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/4] netfilter: Fix portid types
  2015-03-13 11:31 net: portid signedness and format string fixes Richard Weinberger
  2015-03-13 11:31 ` [PATCH 1/4] netlink: Fix portid type in netlink_notify Richard Weinberger
  2015-03-13 11:31 ` [PATCH 2/4] nfc: Fix portid type in urelease_work Richard Weinberger
@ 2015-03-13 11:31 ` Richard Weinberger
  2015-03-13 13:01   ` Pablo Neira Ayuso
  2015-03-13 11:31 ` [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file Richard Weinberger
  3 siblings, 1 reply; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 11:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-wireless, coreteam, netfilter-devel, linux-kernel, sameo,
	aloisio.almeida, lauro.venancio, davem, kadlec, kaber, pablo,
	Richard Weinberger

The netlink portid is an unsigned integer, use this type
also in netfilter.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 net/netfilter/nfnetlink_log.c        | 4 ++--
 net/netfilter/nfnetlink_queue_core.c | 7 ++++---
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
index 11d85b3..7f00846 100644
--- a/net/netfilter/nfnetlink_log.c
+++ b/net/netfilter/nfnetlink_log.c
@@ -62,7 +62,7 @@ struct nfulnl_instance {
 	struct timer_list timer;
 	struct net *net;
 	struct user_namespace *peer_user_ns;	/* User namespace of the peer process */
-	int peer_portid;			/* PORTID of the peer process */
+	unsigned int peer_portid;		/* PORTID of the peer process */
 
 	/* configurable parameters */
 	unsigned int flushtimeout;	/* timeout until queue flush */
@@ -151,7 +151,7 @@ static void nfulnl_timer(unsigned long data);
 
 static struct nfulnl_instance *
 instance_create(struct net *net, u_int16_t group_num,
-		int portid, struct user_namespace *user_ns)
+		unsigned int portid, struct user_namespace *user_ns)
 {
 	struct nfulnl_instance *inst;
 	struct nfnl_log_net *log = nfnl_log_pernet(net);
diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 0db8515..1b7f7b8 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -54,7 +54,7 @@ struct nfqnl_instance {
 	struct hlist_node hlist;		/* global list of queues */
 	struct rcu_head rcu;
 
-	int peer_portid;
+	unsigned int peer_portid;
 	unsigned int queue_maxlen;
 	unsigned int copy_range;
 	unsigned int queue_dropped;
@@ -110,7 +110,7 @@ instance_lookup(struct nfnl_queue_net *q, u_int16_t queue_num)
 
 static struct nfqnl_instance *
 instance_create(struct nfnl_queue_net *q, u_int16_t queue_num,
-		int portid)
+		unsigned int portid)
 {
 	struct nfqnl_instance *inst;
 	unsigned int h;
@@ -860,7 +860,8 @@ static const struct nla_policy nfqa_verdict_batch_policy[NFQA_MAX+1] = {
 };
 
 static struct nfqnl_instance *
-verdict_instance_lookup(struct nfnl_queue_net *q, u16 queue_num, int nlportid)
+verdict_instance_lookup(struct nfnl_queue_net *q, u16 queue_num,
+			unsigned int nlportid)
 {
 	struct nfqnl_instance *queue;
 
-- 
1.8.4.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-13 11:31 net: portid signedness and format string fixes Richard Weinberger
                   ` (2 preceding siblings ...)
  2015-03-13 11:31 ` [PATCH 3/4] netfilter: Fix portid types Richard Weinberger
@ 2015-03-13 11:31 ` Richard Weinberger
  2015-03-13 12:15   ` Pablo Neira Ayuso
  3 siblings, 1 reply; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 11:31 UTC (permalink / raw)
  To: netdev
  Cc: linux-wireless, coreteam, netfilter-devel, linux-kernel, sameo,
	aloisio.almeida, lauro.venancio, davem, kadlec, kaber, pablo,
	Richard Weinberger

The printed values are all of type unsigned integer, therefore use
%u instead of %d. Otherwise an user can face negative values.

Fixes:
$ cat /proc/net/netfilter/nfnetlink_queue
    0  29508   278 2 65531     0 2004213241 -2129885586  1
    1 -27747     0 2 65531     0     0        0  1
    2 -27748     0 2 65531     0     0        0  1

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 net/netfilter/nfnetlink_queue_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
index 1b7f7b8..2121ab5 100644
--- a/net/netfilter/nfnetlink_queue_core.c
+++ b/net/netfilter/nfnetlink_queue_core.c
@@ -1243,7 +1243,7 @@ static int seq_show(struct seq_file *s, void *v)
 {
 	const struct nfqnl_instance *inst = v;
 
-	seq_printf(s, "%5d %6d %5d %1d %5d %5d %5d %8d %2d\n",
+	seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %2d\n",
 		   inst->queue_num,
 		   inst->peer_portid, inst->queue_total,
 		   inst->copy_mode, inst->copy_range,
-- 
1.8.4.5


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-13 11:31 ` [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file Richard Weinberger
@ 2015-03-13 12:15   ` Pablo Neira Ayuso
  2015-03-13 13:43     ` Richard Weinberger
  0 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira Ayuso @ 2015-03-13 12:15 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

On Fri, Mar 13, 2015 at 12:31:16PM +0100, Richard Weinberger wrote:
> The printed values are all of type unsigned integer, therefore use
> %u instead of %d. Otherwise an user can face negative values.
> 
> Fixes:
> $ cat /proc/net/netfilter/nfnetlink_queue
>     0  29508   278 2 65531     0 2004213241 -2129885586  1
>     1 -27747     0 2 65531     0     0        0  1
>     2 -27748     0 2 65531     0     0        0  1

I guess you want to access stats on dropped packets.

I prefer if you extend nfnetlink_queue to provide statistics through
nfnetlink_queue, so you don't have to manually parse this text-based
/proc entry and we can deprecate this interface. That shouldn't have
been there in first place.

Thanks.

> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  net/netfilter/nfnetlink_queue_core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
> index 1b7f7b8..2121ab5 100644
> --- a/net/netfilter/nfnetlink_queue_core.c
> +++ b/net/netfilter/nfnetlink_queue_core.c
> @@ -1243,7 +1243,7 @@ static int seq_show(struct seq_file *s, void *v)
>  {
>  	const struct nfqnl_instance *inst = v;
>  
> -	seq_printf(s, "%5d %6d %5d %1d %5d %5d %5d %8d %2d\n",
> +	seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %2d\n",
>  		   inst->queue_num,
>  		   inst->peer_portid, inst->queue_total,
>  		   inst->copy_mode, inst->copy_range,
> -- 
> 1.8.4.5
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] netfilter: Fix portid types
  2015-03-13 11:31 ` [PATCH 3/4] netfilter: Fix portid types Richard Weinberger
@ 2015-03-13 13:01   ` Pablo Neira Ayuso
  2015-03-13 13:19     ` Richard Weinberger
  0 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira Ayuso @ 2015-03-13 13:01 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

On Fri, Mar 13, 2015 at 12:31:15PM +0100, Richard Weinberger wrote:
> The netlink portid is an unsigned integer, use this type
> also in netfilter.

This small cleanup I can still take it but...

> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
>  net/netfilter/nfnetlink_log.c        | 4 ++--
>  net/netfilter/nfnetlink_queue_core.c | 7 ++++---
>  2 files changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
> index 11d85b3..7f00846 100644
> --- a/net/netfilter/nfnetlink_log.c
> +++ b/net/netfilter/nfnetlink_log.c
> @@ -62,7 +62,7 @@ struct nfulnl_instance {
>  	struct timer_list timer;
>  	struct net *net;
>  	struct user_namespace *peer_user_ns;	/* User namespace of the peer process */
> -	int peer_portid;			/* PORTID of the peer process */
> +	unsigned int peer_portid;		/* PORTID of the peer process */

I think you have to use u32 for consistency. Other spots use the
datatype for the netlink portid.

I think same thing applies to other patches.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] netfilter: Fix portid types
  2015-03-13 13:01   ` Pablo Neira Ayuso
@ 2015-03-13 13:19     ` Richard Weinberger
  0 siblings, 0 replies; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 13:19 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

Am 13.03.2015 um 14:01 schrieb Pablo Neira Ayuso:
> On Fri, Mar 13, 2015 at 12:31:15PM +0100, Richard Weinberger wrote:
>> The netlink portid is an unsigned integer, use this type
>> also in netfilter.
> 
> This small cleanup I can still take it but...
> 
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  net/netfilter/nfnetlink_log.c        | 4 ++--
>>  net/netfilter/nfnetlink_queue_core.c | 7 ++++---
>>  2 files changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/net/netfilter/nfnetlink_log.c b/net/netfilter/nfnetlink_log.c
>> index 11d85b3..7f00846 100644
>> --- a/net/netfilter/nfnetlink_log.c
>> +++ b/net/netfilter/nfnetlink_log.c
>> @@ -62,7 +62,7 @@ struct nfulnl_instance {
>>  	struct timer_list timer;
>>  	struct net *net;
>>  	struct user_namespace *peer_user_ns;	/* User namespace of the peer process */
>> -	int peer_portid;			/* PORTID of the peer process */
>> +	unsigned int peer_portid;		/* PORTID of the peer process */
> 
> I think you have to use u32 for consistency. Other spots use the
> datatype for the netlink portid.
> 
> I think same thing applies to other patches.

Of course! Will re-spin with u32.

Thanks,
//richard

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-13 12:15   ` Pablo Neira Ayuso
@ 2015-03-13 13:43     ` Richard Weinberger
  2015-03-13 13:53       ` Pablo Neira Ayuso
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 13:43 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

Am 13.03.2015 um 13:15 schrieb Pablo Neira Ayuso:
> On Fri, Mar 13, 2015 at 12:31:16PM +0100, Richard Weinberger wrote:
>> The printed values are all of type unsigned integer, therefore use
>> %u instead of %d. Otherwise an user can face negative values.
>>
>> Fixes:
>> $ cat /proc/net/netfilter/nfnetlink_queue
>>     0  29508   278 2 65531     0 2004213241 -2129885586  1
>>     1 -27747     0 2 65531     0     0        0  1
>>     2 -27748     0 2 65531     0     0        0  1
> 
> I guess you want to access stats on dropped packets.

Correct. :)

> I prefer if you extend nfnetlink_queue to provide statistics through
> nfnetlink_queue, so you don't have to manually parse this text-based
> /proc entry and we can deprecate this interface. That shouldn't have
> been there in first place.

You mean statistics via netlink attributes? I can add that!
But I think we should also fix the format string of the proc file
as the fix is easy and non-intrusive.

Thanks,
//richard

> Thanks.
> 
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>>  net/netfilter/nfnetlink_queue_core.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/net/netfilter/nfnetlink_queue_core.c b/net/netfilter/nfnetlink_queue_core.c
>> index 1b7f7b8..2121ab5 100644
>> --- a/net/netfilter/nfnetlink_queue_core.c
>> +++ b/net/netfilter/nfnetlink_queue_core.c
>> @@ -1243,7 +1243,7 @@ static int seq_show(struct seq_file *s, void *v)
>>  {
>>  	const struct nfqnl_instance *inst = v;
>>  
>> -	seq_printf(s, "%5d %6d %5d %1d %5d %5d %5d %8d %2d\n",
>> +	seq_printf(s, "%5u %6u %5u %1u %5u %5u %5u %8u %2d\n",
>>  		   inst->queue_num,
>>  		   inst->peer_portid, inst->queue_total,
>>  		   inst->copy_mode, inst->copy_range,
>> -- 
>> 1.8.4.5
>>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-13 13:43     ` Richard Weinberger
@ 2015-03-13 13:53       ` Pablo Neira Ayuso
  2015-03-13 14:22         ` Richard Weinberger
  0 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira Ayuso @ 2015-03-13 13:53 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

On Fri, Mar 13, 2015 at 02:43:54PM +0100, Richard Weinberger wrote:
> Am 13.03.2015 um 13:15 schrieb Pablo Neira Ayuso:
> > On Fri, Mar 13, 2015 at 12:31:16PM +0100, Richard Weinberger wrote:
> >> The printed values are all of type unsigned integer, therefore use
> >> %u instead of %d. Otherwise an user can face negative values.
> >>
> >> Fixes:
> >> $ cat /proc/net/netfilter/nfnetlink_queue
> >>     0  29508   278 2 65531     0 2004213241 -2129885586  1
> >>     1 -27747     0 2 65531     0     0        0  1
> >>     2 -27748     0 2 65531     0     0        0  1
> > 
> > I guess you want to access stats on dropped packets.
> 
> Correct. :)
> 
> > I prefer if you extend nfnetlink_queue to provide statistics through
> > nfnetlink_queue, so you don't have to manually parse this text-based
> > /proc entry and we can deprecate this interface. That shouldn't have
> > been there in first place.
> 
> You mean statistics via netlink attributes? I can add that!

Add a new NFQNL_CFG_CMD_STATS command to request the statistics. If
NLM_F_DUMP is set, then we'll basically provide the full list of
instances. Otherwise, in case you want to retrieve stats for a
specific netlink socket, you can use the netlink portID as index.
And you'll have to add attributes for this new command, yes.

> But I think we should also fix the format string of the proc file
> as the fix is easy and non-intrusive.

Unfortunately we don't know how many people are relying on that
output, I prefer to remain conservative and provide a proper netlink
interface for this.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-13 13:53       ` Pablo Neira Ayuso
@ 2015-03-13 14:22         ` Richard Weinberger
  2015-03-16 13:11           ` Pablo Neira Ayuso
  0 siblings, 1 reply; 13+ messages in thread
From: Richard Weinberger @ 2015-03-13 14:22 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

Am 13.03.2015 um 14:53 schrieb Pablo Neira Ayuso:
> On Fri, Mar 13, 2015 at 02:43:54PM +0100, Richard Weinberger wrote:
>> Am 13.03.2015 um 13:15 schrieb Pablo Neira Ayuso:
>>> On Fri, Mar 13, 2015 at 12:31:16PM +0100, Richard Weinberger wrote:
>>>> The printed values are all of type unsigned integer, therefore use
>>>> %u instead of %d. Otherwise an user can face negative values.
>>>>
>>>> Fixes:
>>>> $ cat /proc/net/netfilter/nfnetlink_queue
>>>>     0  29508   278 2 65531     0 2004213241 -2129885586  1
>>>>     1 -27747     0 2 65531     0     0        0  1
>>>>     2 -27748     0 2 65531     0     0        0  1
>>>
>>> I guess you want to access stats on dropped packets.
>>
>> Correct. :)
>>
>>> I prefer if you extend nfnetlink_queue to provide statistics through
>>> nfnetlink_queue, so you don't have to manually parse this text-based
>>> /proc entry and we can deprecate this interface. That shouldn't have
>>> been there in first place.
>>
>> You mean statistics via netlink attributes? I can add that!
> 
> Add a new NFQNL_CFG_CMD_STATS command to request the statistics. If
> NLM_F_DUMP is set, then we'll basically provide the full list of
> instances. Otherwise, in case you want to retrieve stats for a
> specific netlink socket, you can use the netlink portID as index.
> And you'll have to add attributes for this new command, yes.

This was my plan. Thanks for the pointer!

>> But I think we should also fix the format string of the proc file
>> as the fix is easy and non-intrusive.
> 
> Unfortunately we don't know how many people are relying on that
> output, I prefer to remain conservative and provide a proper netlink
> interface for this.

I understand your concerns but an application which is able to parse positive
and negative numbers can also parse pure positives.
Just made a small test application, glibc's %d in sscanf() can also deal with UINT_MAX.
And I don't expect that applications to check whether the returned values from
/proc/net/netfilter/nfnetlink_queue are between INT_MIN and INT_MAX.

That said, I'd have assumed that an user would report negative values as plain kernel bug.

Thanks,
//richard

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-13 14:22         ` Richard Weinberger
@ 2015-03-16 13:11           ` Pablo Neira Ayuso
  2015-04-09 21:58             ` Richard Weinberger
  0 siblings, 1 reply; 13+ messages in thread
From: Pablo Neira Ayuso @ 2015-03-16 13:11 UTC (permalink / raw)
  To: Richard Weinberger
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

On Fri, Mar 13, 2015 at 03:22:07PM +0100, Richard Weinberger wrote:
> Am 13.03.2015 um 14:53 schrieb Pablo Neira Ayuso:
> >> You mean statistics via netlink attributes? I can add that!
> > 
> > Add a new NFQNL_CFG_CMD_STATS command to request the statistics. If
> > NLM_F_DUMP is set, then we'll basically provide the full list of
> > instances. Otherwise, in case you want to retrieve stats for a
> > specific netlink socket, you can use the netlink portID as index.
> > And you'll have to add attributes for this new command, yes.
> 
> This was my plan. Thanks for the pointer!

It would be great if you can contribute this new interface.

> >> But I think we should also fix the format string of the proc file
> >> as the fix is easy and non-intrusive.
> > 
> > Unfortunately we don't know how many people are relying on that
> > output, I prefer to remain conservative and provide a proper netlink
> > interface for this.
> 
> I understand your concerns but an application which is able to parse positive
> and negative numbers can also parse pure positives.
> Just made a small test application, glibc's %d in sscanf() can also deal with UINT_MAX.
> And I don't expect that applications to check whether the returned values from
> /proc/net/netfilter/nfnetlink_queue are between INT_MIN and INT_MAX.
> 
> That said, I'd have assumed that an user would report negative values as plain kernel bug.

Makes sense, please fix net/netfilter/nfnetlink_log.c too.

Thanks.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file
  2015-03-16 13:11           ` Pablo Neira Ayuso
@ 2015-04-09 21:58             ` Richard Weinberger
  0 siblings, 0 replies; 13+ messages in thread
From: Richard Weinberger @ 2015-04-09 21:58 UTC (permalink / raw)
  To: Pablo Neira Ayuso
  Cc: netdev, linux-wireless, coreteam, netfilter-devel, linux-kernel,
	sameo, aloisio.almeida, lauro.venancio, davem, kadlec, kaber

Am 16.03.2015 um 14:11 schrieb Pablo Neira Ayuso:
> On Fri, Mar 13, 2015 at 03:22:07PM +0100, Richard Weinberger wrote:
>> Am 13.03.2015 um 14:53 schrieb Pablo Neira Ayuso:
>>>> You mean statistics via netlink attributes? I can add that!
>>>
>>> Add a new NFQNL_CFG_CMD_STATS command to request the statistics. If
>>> NLM_F_DUMP is set, then we'll basically provide the full list of
>>> instances. Otherwise, in case you want to retrieve stats for a
>>> specific netlink socket, you can use the netlink portID as index.
>>> And you'll have to add attributes for this new command, yes.
>>
>> This was my plan. Thanks for the pointer!
> 
> It would be great if you can contribute this new interface.

FYI, it is still on my TODO.
I fear I won't find the time to do a patch for the upcoming merge window
and it has to wait for v4.2.

>>>> But I think we should also fix the format string of the proc file
>>>> as the fix is easy and non-intrusive.
>>>
>>> Unfortunately we don't know how many people are relying on that
>>> output, I prefer to remain conservative and provide a proper netlink
>>> interface for this.
>>
>> I understand your concerns but an application which is able to parse positive
>> and negative numbers can also parse pure positives.
>> Just made a small test application, glibc's %d in sscanf() can also deal with UINT_MAX.
>> And I don't expect that applications to check whether the returned values from
>> /proc/net/netfilter/nfnetlink_queue are between INT_MIN and INT_MAX.
>>
>> That said, I'd have assumed that an user would report negative values as plain kernel bug.
> 
> Makes sense, please fix net/netfilter/nfnetlink_log.c too.

Patches sent! :)

Thanks,
//richard

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2015-04-09 21:58 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-03-13 11:31 net: portid signedness and format string fixes Richard Weinberger
2015-03-13 11:31 ` [PATCH 1/4] netlink: Fix portid type in netlink_notify Richard Weinberger
2015-03-13 11:31 ` [PATCH 2/4] nfc: Fix portid type in urelease_work Richard Weinberger
2015-03-13 11:31 ` [PATCH 3/4] netfilter: Fix portid types Richard Weinberger
2015-03-13 13:01   ` Pablo Neira Ayuso
2015-03-13 13:19     ` Richard Weinberger
2015-03-13 11:31 ` [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file Richard Weinberger
2015-03-13 12:15   ` Pablo Neira Ayuso
2015-03-13 13:43     ` Richard Weinberger
2015-03-13 13:53       ` Pablo Neira Ayuso
2015-03-13 14:22         ` Richard Weinberger
2015-03-16 13:11           ` Pablo Neira Ayuso
2015-04-09 21:58             ` Richard Weinberger

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).