Hi Matthew

-----Original Message-----
From: Matthew Wilcox <willy@infradead.org>
Sent: Friday, May 28, 2021 11:22 PM
To: Justin He <redacted>
Cc: Linus Torvalds <torvalds@linux-foundation.org>; Petr Mladek
[off-list ref]; Steven Rostedt [off-list ref]; Sergey
Senozhatsky [off-list ref]; Andy Shevchenko
[off-list ref]; Rasmus Villemoes
[off-list ref]; Jonathan Corbet [off-list ref]; Alexander
Viro [off-list ref]; Luca Coelho [off-list ref];
Kalle Valo [off-list ref]; David S. Miller [off-list ref];
Jakub Kicinski [off-list ref]; Heiko Carstens [off-list ref];
Vasily Gorbik [off-list ref]; Christian Borntraeger
[off-list ref]; Johannes Berg [off-list ref]; linux-
doc@vger.kernel.org; linux-kernel@vger.kernel.org; linux-
wireless@vger.kernel.org; netdev@vger.kernel.org; linux-
s390@vger.kernel.org
Subject: Re: [PATCH RFCv2 2/3] lib/vsprintf.c: make %pD print full path
for file

On Fri, May 28, 2021 at 03:09:28PM +0000, Justin He wrote:

quoted

quoted

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.

quoted

We need another new helper __prepend_path_size() to get the full path

size

quoted

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

What do you think of it?

--
Cheers,
Justin (Jia He)

quoted

More than that, even the 1st vsnprintf could have _end_ > _buf_ in some

case:

quoted

What if printk("%pD", filp) ? The 1st vsnprintf has positive (end-buf).

I don't understand the problem ... if p->len is positive, then you
succeeded.  if p->len is negative then -p->len is the expected return
value from vsnprintf().  No?

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.