| Message ID | 1426246276-15839-5-git-send-email-richard@nod.at |
|---|---|
| State | Changes Requested |
| Delegated to: | Pablo Neira |
| Headers | show |
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 > -- 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
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 >> -- 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
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. -- 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
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 -- 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
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. -- 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
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 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 --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,
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(-)