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(-)
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@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)
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@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@meshcoding.com
Thanks,
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,
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...
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.
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.
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:
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./
Yeah, I started to use true/false, 0/1, but thought I needed to match what seq_printf/seq_vprintf does.
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; }
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:
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./
Yeah, I started to use true/false, 0/1, but thought I needed to match what seq_printf/seq_vprintf does.
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; }
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.
From: Joe Perches joe@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@davemloft.net
b.a.t.m.a.n@lists.open-mesh.org
-
Al Viro -
Antonio Quartulli -
David Miller -
Joe Perches