linux-kernel - Re: [PATCH v2 1/4] fs: introduce helper d_path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 23 Jun 2021 12:10:37 +0300
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     Jia He <justin.he@....com>
Cc:     Petr Mladek <pmladek@...e.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Sergey Senozhatsky <senozhatsky@...omium.org>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        Jonathan Corbet <corbet@....net>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Eric Biggers <ebiggers@...gle.com>,
        "Ahmed S. Darwish" <a.darwish@...utronix.de>,
        linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        Matthew Wilcox <willy@...radead.org>,
        Christoph Hellwig <hch@...radead.org>, nd@....com
Subject: Re: [PATCH v2 1/4] fs: introduce helper d_path_unsafe()

On Wed, Jun 23, 2021 at 01:50:08PM +0800, Jia He wrote:
> This helper is similar to d_path() except that it doesn't take any
> seqlock/spinlock. It is typical for debugging purposes. Besides,
> an additional return value *prenpend_len* is used to get the full
> path length of the dentry, ingoring the tail '\0'.
> the full path length = end - buf - prepend_length - 1.
> 
> Previously it will skip the prepend_name() loop at once in
> __prepen_path() when the buffer length is not enough or even negative.
> prepend_name_with_len() will get the full length of dentry name
> together with the parent recursively regardless of the buffer length.

...

>  /**
>   * prepend_name - prepend a pathname in front of current buffer pointer
> - * @buffer: buffer pointer
> - * @buflen: allocated length of the buffer
> - * @name:   name string and length qstr structure
> + * @p: prepend buffer which contains buffer pointer and allocated length
> + * @name: name string and length qstr structure
>   *
>   * With RCU path tracing, it may race with d_move(). Use READ_ONCE() to
>   * make sure that either the old or the new name pointer and length are

This should be separate patch. You are sending new version too fast...
Instead of speeding up it will slow down the review process.

...

> +	const char *dname = smp_load_acquire(&name->name); /* ^^^ */

I have commented on the comment here. What does it mean for mere reader?

> +	int dlen = READ_ONCE(name->len);
> +	char *s;
> +	int last_len = p->len;

Reversed xmas tree order, please.

The rule of thumb is when you have gotten a comment against a piece of code,
try to fix all similar places at once.

...

> @@ -108,8 +181,7 @@ static int __prepend_path(const struct dentry *dentry, const struct mount *mnt,
>   * prepend_path - Prepend path string to a buffer
>   * @path: the dentry/vfsmount to report
>   * @root: root vfsmnt/dentry
> - * @buffer: pointer to the end of the buffer
> - * @buflen: pointer to buffer length
> + * @p: prepend buffer which contains buffer pointer and allocated length
>   *
>   * The function will first try to write out the pathname without taking any
>   * lock other than the RCU read lock to make sure that dentries won't go away.

Kernel doc fix should be in a separate patch.


-- 
With Best Regards,
Andy Shevchenko