| Message ID | e7104b35287b1e1ec456734c1b5e1aa98dac99f8.1386738050.git.joe@perches.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
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 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" 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/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)
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(-)