[B.A.T.M.A.N.] [PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void

[B.A.T.M.A.N.] [PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void

[Joe Perches]

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

[B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Joe Perches]

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)
-- 
1.8.1.2.459.gbcd45b4.dirty

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 621 bytes --]

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,

-- 
Antonio Quartulli


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Antonio Quartulli]

[-- Attachment #1: Type: text/plain, Size: 275 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Al Viro]

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

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Al Viro]

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.

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Al Viro]

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.

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Joe Perches]

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

Re: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow

[Al Viro]

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.

Re: [B.A.T.M.A.N.] [PATCH -next 0/3] seq_printf/puts/putc: Start to convert to return void

[David Miller]

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>