net: portid signedness and format string fixes

net: portid signedness and format string fixes

[Richard Weinberger]

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

[PATCH 1/4] netlink: Fix portid type in netlink_notify

[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

[PATCH 2/4] nfc: Fix portid type in urelease_work

[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

[PATCH 3/4] netfilter: Fix portid types

[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

Re: [PATCH 3/4] netfilter: Fix portid types

[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.

Re: [PATCH 3/4] netfilter: Fix portid types

[Richard Weinberger]

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

[PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[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

Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[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.

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
> 

Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[Richard Weinberger]

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
>>

Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[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.

> 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.

Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[Richard Weinberger]

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

Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[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.

> >> 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.

Re: [PATCH 4/4] netfilter: Fix format string of nfnetlink_queue proc file

[Richard Weinberger]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html