11 Dec
2013
11 Dec
'13
5:12 a.m.
Many uses of the return value of seq_printf/seq_puts/seq_putc are
incorrect. Many assume that the return value is the number of
chars emitted into a buffer like printf/puts/putc.
It would be better to make the return value of these functions void
to avoid these misuses.
Start to do so.
Convert seq_overflow to a public function from a static function.
Remove the return uses of seq_printf/seq_puts/seq_putc from net.
Add a seq_overflow function call instead.
Joe Perches (3):
seq: Add a seq_overflow test.
batman-adv: Use seq_overflow
netfilter: Use seq_overflow
fs/seq_file.c | 15 ++++----
include/linux/seq_file.h | 2 +
include/net/netfilter/nf_conntrack_acct.h | 3 +-
net/batman-adv/gateway_client.c | 25 ++++++------
net/ipv4/netfilter/nf_conntrack_l3proto_ipv4.c | 6 ++-
.../netfilter/nf_conntrack_l3proto_ipv4_compat.c | 42 +++++++++++++--------
net/ipv6/netfilter/nf_conntrack_l3proto_ipv6.c | 6 ++-
net/ipv6/netfilter/nf_conntrack_proto_icmpv6.c | 10 +++--
net/netfilter/nf_conntrack_acct.c | 11 +++---
net/netfilter/nf_conntrack_expect.c | 4 +-
net/netfilter/nf_conntrack_proto_dccp.c | 12 ++++--
net/netfilter/nf_conntrack_proto_gre.c | 15 +++++---
net/netfilter/nf_conntrack_proto_sctp.c | 12 ++++--
net/netfilter/nf_conntrack_proto_tcp.c | 11 ++++--
net/netfilter/nf_conntrack_proto_udp.c | 7 ++--
net/netfilter/nf_conntrack_proto_udplite.c | 7 ++--
net/netfilter/nf_conntrack_standalone.c | 44 +++++++++++++---------
net/netfilter/nf_log.c | 26 ++++++-------
net/netfilter/nfnetlink_log.c | 12 +++---
net/netfilter/nfnetlink_queue_core.c | 14 ++++---
net/netfilter/x_tables.c | 8 ++--
net/netfilter/xt_hashlimit.c | 34 +++++++++--------
22 files changed, 191 insertions(+), 135 deletions(-)
--
1.8.1.2.459.gbcd45b4.dirty
11 Dec
11 Dec
5:12 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
Convert the uses of the return of seq_printf to
instead check seq_overflow to determine if a buffer
overflow has occurred.
This will eventually allow seq_printf & seq_puts to
be converted to a void return instead of the often
misused return that is often assumed to be an int for
the number of bytes emitted ala printk.
Signed-off-by: Joe Perches <joe(a)perches.com>
---
net/batman-adv/gateway_client.c | 25 ++++++++++++-------------
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/net/batman-adv/gateway_client.c b/net/batman-adv/gateway_client.c
index 2449afa..dfa5d2d 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@ -517,29 +517,28 @@ static int batadv_write_buffer_text(struct batadv_priv *bat_priv,
{
struct batadv_gw_node *curr_gw;
struct batadv_neigh_node *router;
- int ret = -1;
router = batadv_orig_node_get_router(gw_node->orig_node);
if (!router)
- goto out;
+ return -1;
curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
- ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %u.%u/%u.%u MBit\n",
- (curr_gw == gw_node ? "=>" : " "),
- gw_node->orig_node->orig,
- router->bat_iv.tq_avg, router->addr,
- router->if_incoming->net_dev->name,
- gw_node->bandwidth_down / 10,
- gw_node->bandwidth_down % 10,
- gw_node->bandwidth_up / 10,
- gw_node->bandwidth_up % 10);
+ seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %u.%u/%u.%u MBit\n",
+ (curr_gw == gw_node ? "=>" : " "),
+ gw_node->orig_node->orig,
+ router->bat_iv.tq_avg, router->addr,
+ router->if_incoming->net_dev->name,
+ gw_node->bandwidth_down / 10,
+ gw_node->bandwidth_down % 10,
+ gw_node->bandwidth_up / 10,
+ gw_node->bandwidth_up % 10);
batadv_neigh_node_free_ref(router);
if (curr_gw)
batadv_gw_node_free_ref(curr_gw);
-out:
- return ret;
+
+ return seq_overflow(seq);
}
int batadv_gw_client_seq_print_text(struct seq_file *seq, void *offset)
--
1.8.1.2.459.gbcd45b4.dirty
7:26 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
On 11/12/13 06:12, Joe Perches wrote:
Convert the uses of the return of seq_printf to
instead check seq_overflow to determine if a buffer
overflow has occurred.
This will eventually allow seq_printf & seq_puts to
be converted to a void return instead of the often
misused return that is often assumed to be an int for
the number of bytes emitted ala printk.
Signed-off-by: Joe Perches <joe(a)perches.com>
I assume this patch is going to be merged with the others in some tree.
In that case:
Acked-by: Antonio Quartulli <antonio(a)meshcoding.com>
Thanks,
--
Antonio Quartulli
7:31 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
Joe,
we have other places in the batman-adv code where we use seq_printf, but
at the moment we don't check the return value and we always return 0 at
the end of the function.
I think we could use seq_overflow here as well?
Thanks,
--
Antonio Quartulli
7:58 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
On Wed, Dec 11, 2013 at 08:31:35AM +0100, Antonio Quartulli wrote:
Joe,
we have other places in the batman-adv code where we use seq_printf, but
at the moment we don't check the return value and we always return 0 at
the end of the function.
I think we could use seq_overflow here as well?
Not if you want correctly behaving code...
7:55 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
On Tue, Dec 10, 2013 at 09:12:43PM -0800, Joe Perches wrote:
diff --git a/net/batman-adv/gateway_client.c
b/net/batman-adv/gateway_client.c
index 2449afa..dfa5d2d 100644
--- a/net/batman-adv/gateway_client.c
+++ b/net/batman-adv/gateway_client.c
@@ -517,29 +517,28 @@ static int batadv_write_buffer_text(struct batadv_priv *bat_priv,
{
struct batadv_gw_node *curr_gw;
struct batadv_neigh_node *router;
- int ret = -1;
router = batadv_orig_node_get_router(gw_node->orig_node);
if (!router)
- goto out;
+ return -1;
This (as well as the original) means "fail read(2) with -EINVAL", which
might or might not be correct behaviour.
curr_gw = batadv_gw_get_selected_gw_node(bat_priv);
- ret = seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %u.%u/%u.%u MBit\n",
- (curr_gw == gw_node ? "=>" : " "),
- gw_node->orig_node->orig,
- router->bat_iv.tq_avg, router->addr,
- router->if_incoming->net_dev->name,
- gw_node->bandwidth_down / 10,
- gw_node->bandwidth_down % 10,
- gw_node->bandwidth_up / 10,
- gw_node->bandwidth_up % 10);
+ seq_printf(seq, "%s %pM (%3i) %pM [%10s]: %u.%u/%u.%u MBit\n",
+ (curr_gw == gw_node ? "=>" : " "),
+ gw_node->orig_node->orig,
+ router->bat_iv.tq_avg, router->addr,
+ router->if_incoming->net_dev->name,
+ gw_node->bandwidth_down / 10,
+ gw_node->bandwidth_down % 10,
+ gw_node->bandwidth_up / 10,
+ gw_node->bandwidth_up % 10);
batadv_neigh_node_free_ref(router);
if (curr_gw)
batadv_gw_node_free_ref(curr_gw);
-out:
- return ret;
+
+ return seq_overflow(seq);
... and this is utter junk.
This sucker should return 0. Insufficiently large buffer will be handled
by caller, TYVM, if you give that caller a chance to do so. Returning 1
from ->show() is a bug in almost all cases, and definitely so in this one.
Just in case somebody decides that above is worth copying: It Is Not.
Original code is buggy, plain and simple. This one trades the older
bug ("fail with -EINVAL whenever the buffer is too small") with just as buggy
"silently skip an entry entirely whenever the buffer is too small".
Don't Do That.
8:05 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
On Wed, Dec 11, 2013 at 07:55:26AM +0000, Al Viro wrote:
This sucker should return 0. Insufficiently large
buffer will be handled
by caller, TYVM, if you give that caller a chance to do so. Returning 1
from ->show() is a bug in almost all cases, and definitely so in this one.
Just in case somebody decides that above is worth copying: It Is Not.
Original code is buggy, plain and simple. This one trades the older
bug ("fail with -EINVAL whenever the buffer is too small") with just as buggy
"silently skip an entry entirely whenever the buffer is too small".
Don't Do That.
Pardon - Joe has made seq_overflow return -1 instead of true. Correction
to the above, then - s/This trades.*\./This is just as buggy./
Conclusion is still the same - Don't Do That. Returning -1 on insufficiently
large buffer is a bug, plain and simple.
And this patch series is completely misguided - it doesn't fix any bugs
*and* it provides a misleading example for everyone. See the reaction
right in this thread, proposing to spread the same bug to currently
working iterators.
8:26 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
On Wed, 2013-12-11 at 08:05 +0000, Al Viro wrote:
On Wed, Dec 11, 2013 at 07:55:26AM +0000, Al Viro
wrote:
Yeah, I started to use true/false, 0/1, but thought
I needed to match what seq_printf/seq_vprintf does.
This sucker should return 0. Insufficiently
large buffer will be handled
by caller, TYVM, if you give that caller a chance to do so. Returning 1
from ->show() is a bug in almost all cases, and definitely so in this one.
Just in case somebody decides that above is worth copying: It Is Not.
Original code is buggy, plain and simple. This one trades the older
bug ("fail with -EINVAL whenever the buffer is too small") with just as buggy
"silently skip an entry entirely whenever the buffer is too small".
Don't Do That.
Pardon - Joe has made seq_overflow return -1 instead of true. Correction
to the above, then - s/This trades.*\./This is just as buggy./ Conclusion is still the same - Don't Do That.
Returning -1 on insufficiently
large buffer is a bug, plain and simple.
int seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
int len;
if (m->count < m->size) {
len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
if (m->count + len < m->size) {
m->count += len;
return 0;
}
}
seq_set_overflow(m);
return -1;
}
EXPORT_SYMBOL(seq_vprintf);
int seq_printf(struct seq_file *m, const char *f, ...)
{
int ret;
va_list args;
va_start(args, f);
ret = seq_vprintf(m, f, args);
va_end(args);
return ret;
}
EXPORT_SYMBOL(seq_printf);
And this patch series is completely misguided - it
doesn't fix any bugs
*and* it provides a misleading example for everyone. See the reaction
right in this thread, proposing to spread the same bug to currently
working iterators.
Anyway, changing seq_overflow is easy enough
You prefer this?
bool seq_overflow(struct seq_file *seq)
{
return m->count == m->size;
}
8:38 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow
On Wed, Dec 11, 2013 at 12:26:17AM -0800, Joe Perches wrote:
On Wed, 2013-12-11 at 08:05 +0000, Al Viro wrote:
I prefer a series that starts with fixing the obvious bugs (i.e. places
where we return seq_printf/seq_puts/seq_putc return value from ->show()).
All such places should return 0. Then we need to look at the remaining
places that check return value of seq_printf() et.al. And decide whether
the callers really care about it.
Theoretically, there is a legitimate case when we want to look at that
return value. Namely,
seq_print(...)
if (!overflowed)
do tons of expensive calculations
generate more output
return 0
That is the reason why those guys hadn't been returning void to start with.
And yes, it was inviting bugs with ->show() returning -1 on overflows.
Bad API design, plain and simple.
I'm not sure we actually have any instances of that legitimate case, TBH.
_IF_ we do, we ought to expose seq_overflow() (with saner name - this one
invites the same "it's an error, need to report it" kind of bugs) and use
it in such places. But that needs to be decided on per-caller basis. And
I'd expect that there would be few enough such places after we kill the
obvious bugs.
On Wed, Dec 11, 2013 at 07:55:26AM +0000, Al Viro
wrote:
Yeah, I started to use true/false, 0/1, but thought
I needed to match what seq_printf/seq_vprintf does.
This sucker should return 0. Insufficiently
large buffer will be handled
by caller, TYVM, if you give that caller a chance to do so. Returning 1
from ->show() is a bug in almost all cases, and definitely so in this one.
Just in case somebody decides that above is worth copying: It Is Not.
Original code is buggy, plain and simple. This one trades the older
bug ("fail with -EINVAL whenever the buffer is too small") with just as buggy
"silently skip an entry entirely whenever the buffer is too small".
Don't Do That.
Pardon - Joe has made seq_overflow return -1 instead of true. Correction
to the above, then - s/This trades.*\./This is just as buggy./ Conclusion is still the same - Don't Do That.
Returning -1 on insufficiently
large buffer is a bug, plain and simple.
int seq_vprintf(struct seq_file *m, const char *f, va_list args)
{
int len;
if (m->count < m->size) {
len = vsnprintf(m->buf + m->count, m->size - m->count, f, args);
if (m->count + len < m->size) {
m->count += len;
return 0;
}
}
seq_set_overflow(m);
return -1;
}
EXPORT_SYMBOL(seq_vprintf);
int seq_printf(struct seq_file *m, const char *f, ...)
{
int ret;
va_list args;
va_start(args, f);
ret = seq_vprintf(m, f, args);
va_end(args);
return ret;
}
EXPORT_SYMBOL(seq_printf);
And this patch series is completely misguided -
it doesn't fix any bugs
*and* it provides a misleading example for everyone. See the reaction
right in this thread, proposing to spread the same bug to currently
working iterators.
Anyway, changing seq_overflow is easy enough
You prefer this?
bool seq_overflow(struct seq_file *seq)
{
return m->count == m->size;
} 
5:20 a.m.
New subject: [B.A.T.M.A.N.] [PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void
From: Joe Perches <joe(a)perches.com>
Date: Tue, 10 Dec 2013 21:12:41 -0800
Many uses of the return value of
seq_printf/seq_puts/seq_putc are
incorrect. Many assume that the return value is the number of
chars emitted into a buffer like printf/puts/putc.
It would be better to make the return value of these functions void
to avoid these misuses.
Start to do so.
Convert seq_overflow to a public function from a static function.
Remove the return uses of seq_printf/seq_puts/seq_putc from net.
Add a seq_overflow function call instead.
I'm fine with this going in whatever tree is appropriate for
the seq_overflow un-static change:
Acked-by: David S. Miller <davem(a)davemloft.net>
3461
days inactive
3461
days old
b.a.t.m.a.n@lists.open-mesh.org
9 comments
4 participants
participants (4)
-
Al Viro -
Antonio Quartulli -
David Miller -
Joe Perches