Message ID | e7104b35287b1e1ec456734c1b5e1aa98dac99f8.1386738050.git.joe@perches.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-Path: <joe@perches.com> Received: from smtprelay.hostedemail.com (smtprelay0127.hostedemail.com [216.40.44.127]) by open-mesh.org (Postfix) with ESMTP id EF0906022FA for <b.a.t.m.a.n@lists.open-mesh.org>; Wed, 11 Dec 2013 06:12:58 +0100 (CET) Received: from filter.hostedemail.com (ff-bigip1 [10.5.19.254]) by smtprelay03.hostedemail.com (Postfix) with ESMTP id 104DE6B7B4; Wed, 11 Dec 2013 05:12:58 +0000 (UTC) X-Session-Marker: 6A6F6540706572636865732E636F6D X-Spam-Summary: 48, 2.914, 0, , d41d8cd98f00b204, joe@perches.com, :::::::::::::::::, RULES_HIT:41:69:355:379:418:541:800:960:966:973:988:989:1260:1345:1359:1437:1534:1542:1711:1730:1747:1777:1792:2196:2199:2393:2559:2562:2693:3138:3139:3140:3141:3142:3353:3865:3866:3867:3871:3874:4385:5007:6261:10004:10026:10848:11026:11658:11914:12043:12296:12438:12517:12519:12555:13161:13229, 0, RBL:none, CacheIP:none, Bayesian:0.5, 0.5, 0.5, Netcheck:none, DomainCache:0, MSF:not bulk, SPF:fn, MSBL:0, DNSBL:none, Custom_rules:0:0:0 X-HE-Tag: glue61_70f6566cf5d54 X-Filterd-Recvd-Size: 3020 Received: from localhost.localdomain (pool-96-251-49-11.lsanca.fios.verizon.net [96.251.49.11]) (Authenticated sender: joe@perches.com) by omf12.hostedemail.com (Postfix) with ESMTPA; Wed, 11 Dec 2013 05:12:56 +0000 (UTC) From: Joe Perches <joe@perches.com> To: Alexander Viro <viro@zeniv.linux.org.uk> Date: Tue, 10 Dec 2013 21:12:43 -0800 Message-Id: <e7104b35287b1e1ec456734c1b5e1aa98dac99f8.1386738050.git.joe@perches.com> X-Mailer: git-send-email 1.8.1.2.459.gbcd45b4.dirty In-Reply-To: <cover.1386738050.git.joe@perches.com> References: <cover.1386738050.git.joe@perches.com> Cc: Kees Cook <keescook@chromium.org>, netdev@vger.kernel.org, b.a.t.m.a.n@lists.open-mesh.org, linux-kernel@vger.kernel.org, Antonio Quartulli <antonio@meshcoding.com>, "David S. Miller" <davem@davemloft.net>, Marek Lindner <mareklindner@neomailbox.ch> Subject: [B.A.T.M.A.N.] [PATCH -next 2/3] batman-adv: Use seq_overflow X-BeenThere: b.a.t.m.a.n@lists.open-mesh.org X-Mailman-Version: 2.1.15 Precedence: list Reply-To: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n@lists.open-mesh.org> List-Id: The list for a Better Approach To Mobile Ad-hoc Networking <b.a.t.m.a.n.lists.open-mesh.org> List-Unsubscribe: <https://lists.open-mesh.org/mm/options/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=unsubscribe> List-Archive: <http://lists.open-mesh.org/pipermail/b.a.t.m.a.n/> List-Post: <mailto:b.a.t.m.a.n@lists.open-mesh.org> List-Help: <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=help> List-Subscribe: <https://lists.open-mesh.org/mm/listinfo/b.a.t.m.a.n>, <mailto:b.a.t.m.a.n-request@lists.open-mesh.org?subject=subscribe> X-List-Received-Date: Wed, 11 Dec 2013 05:12:59 -0000 |
Commit Message
Joe Perches
Dec. 11, 2013, 5:12 a.m. UTC
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(-)
Comments
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.
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 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.
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)