From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8B225C47093 for ; Tue, 1 Jun 2021 15:31:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 7234A613AB for ; Tue, 1 Jun 2021 15:31:40 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234368AbhFAPdT (ORCPT ); Tue, 1 Jun 2021 11:33:19 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:40566 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234305AbhFAPdO (ORCPT ); Tue, 1 Jun 2021 11:33:14 -0400 Received: from casper.infradead.org (casper.infradead.org [IPv6:2001:8b0:10b:1236::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0A931C061574; Tue, 1 Jun 2021 08:31:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=spulMJU5vcfm6fCDiXKafJqkb0QFdQy2mPq+FRg3HrI=; b=MutdgsODQUN/W4vU1lKs+wXA2y z9fs2I5gVYi629eoU7OElZjgZFcyo/GhXq4Zp14HO7usWO1hYc45clLTQF4nLnVk1iMLyMX+mo6J4 n99BeytS6B7UaSG/IBtoVBbH0lasTiVpvs5VXWnRwgTUlMCEKD5C3xJscexDf0op9JU/Rik9RYBj0 jOtkW8/sivy6F+MeQA3q2xG8oy8nGdtJpsSKR6F+lrBngwSjHTYbK2SEeK8Q73GdMMOpamtPKICBZ YYCYK961/27+L3Kh4Id08WqDV2W0Gw7ZHT0gtgyZar+YsJzJjdk2GEQ3mXh4b8WuwxHEcEZiNW5Kl 161ausGQ==; Received: from willy by casper.infradead.org with local (Exim 4.94 #2 (Red Hat Linux)) id 1lo6LJ-00AA26-Vr; Tue, 01 Jun 2021 15:30:15 +0000 Date: Tue, 1 Jun 2021 16:30:09 +0100 From: Matthew Wilcox To: Justin He Cc: Linus Torvalds , Petr Mladek , Steven Rostedt , Sergey Senozhatsky , Andy Shevchenko , Rasmus Villemoes , Jonathan Corbet , Alexander Viro , Luca Coelho , Kalle Valo , "David S. Miller" , Jakub Kicinski , Heiko Carstens , Vasily Gorbik , Christian Borntraeger , Johannes Berg , "linux-doc@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-wireless@vger.kernel.org" , "netdev@vger.kernel.org" , "linux-s390@vger.kernel.org" , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH RFCv2 2/3] lib/vsprintf.c: make %pD print full path for file Message-ID: References: <20210528113951.6225-1-justin.he@arm.com> <20210528113951.6225-3-justin.he@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org somehow the linux-fsdevel mailing list got dropped from this revision of the patch set. anyone who's following along may wish to refer to the archives: https://lore.kernel.org/linux-doc/20210528113951.6225-1-justin.he@arm.com/ On Tue, Jun 01, 2021 at 02:42:15PM +0000, Justin He wrote: > > On Fri, May 28, 2021 at 03:09:28PM +0000, Justin He wrote: > > > > I'm not sure why it's so complicated. p->len records how many bytes > > > > are needed for the entire path; can't you just return -p->len ? > > > > > > prepend_name() will return at the beginning if p->len is <0 in this case, > > > we can't even get the correct full path size if keep __prepend_path > > unchanged. > > > We need another new helper __prepend_path_size() to get the full path > > size > > > regardless of the negative value p->len. > > > > It's a little hard to follow, based on just the patches. Is there a > > git tree somewhere of Al's patches that you're based on? > > > > Seems to me that prepend_name() is just fine because it updates p->len > > before returning false: > > > > static bool prepend_name(struct prepend_buffer *p, const struct qstr > > *name) > > { > > const char *dname = smp_load_acquire(&name->name); /* ^^^ */ > > u32 dlen = READ_ONCE(name->len); > > char *s; > > > > p->len -= dlen + 1; > > if (unlikely(p->len < 0)) > > return false; > > > > I think the only change you'd need to make for vsnprintf() is in > > prepend_path(): > > > > - if (!prepend_name(&b, &dentry->d_name)) > > - break; > > + prepend_name(&b, &dentry->d_name); > > > > Would that hurt anything else? > > > > It almost works except the snprintf case, > Consider,assuming filp path is 256 bytes, 2 dentries "/root/$long_string": > snprintf(buffer, 128, "%pD", filp); > p->len is positive at first, but negative after prepend_name loop. > So, it will not fill any bytes in _buffer_. > But in theory, it should fill the beginning 127 bytes and '\0'. I have a few thoughts ... 1. Do we actually depend on that anywhere? 2. Is that something we should support? 3. We could print the start of the filename, if we do. So something like this ... static void prepend(struct prepend_buffer *p, const char *str, int namelen) { p->len -= namelen; if (likely(p->len >= 0)) { p->buf -= namelen; memcpy(p->buf, str, namelen); } else { char *s = p->buf; int buflen = strlen(p->buf); /* The first time we overflow the buffer */ if (p->len + namelen > 0) { p->buf -= p->len + namelen; buflen += p->len + namelen; } if (buflen > namelen) { memmove(p->buf + namelen, s, buflen - namelen); memcpy(p->buf, str, namelen); } else { memcpy(p->buf, str, buflen); } } } I haven't tested this; it's probably full of confusion and off-by-one errors. But I hope you get the point -- we continue to accumulate p->len to indicate how many characters we shifted off the right of the buffer while adding the (start of) the filename on the left. 4. If we want the end of the filename instead, that looks easier: static void prepend(struct prepend_buffer *p, const char *str, int namelen) { p->len -= namelen; if (likely(p->len >= 0)) { p->buf -= namelen; memcpy(p->buf, str, namelen); } else if (p->len + namelen > 0) { p->buf -= p->len + namelen; memcpy(p->buf, str - p->len, p->len + namelen) } } But I don't think we want any of this at all. Just don't put anything in the buffer if the user didn't supply enough space. As long as you get the return value right, they know the string is bad (or they don't care if the string is bad)