Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX

On Tue, 2024-11-19 at 13:38 +0100, Ilya Dryomov wrote:

On Mon, Nov 18, 2024 at 11:28 PM Max Kellermann max.kellermann@ionos.com wrote:

...

If the full path to be built by ceph_mdsc_build_path() happens to be longer than PATH_MAX, then this function will enter an endless (retry) loop, effectively blocking the whole task. Most of the machine becomes unusable, making this a very simple and effective DoS vulnerability.

I cannot imagine why this retry was ever implemented, but it seems rather useless and harmful to me. Let's remove it and fail with ENAMETOOLONG instead.

Hi Max,

When this was put in place in 2009, I think the idea of a retry was copied from CIFS. Jeff preserved the retry when he massaged this code to not warn in case a rename race is detected [1]. CIFS got rid of it only a couple of years ago [2][3].

Adding Patrick and Venky as well, please chime in.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i... [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?i...

Thanks,

            Ilya

...

Cc: stable@vger.kernel.org Reported-by: Dario Weißer dario@cure53.de Signed-off-by: Max Kellermann max.kellermann@ionos.com

---

fs/ceph/mds_client.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index c4a5fd94bbbb..4f6ac015edcd 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -2808,12 +2808,11 @@ char *ceph_mdsc_build_path(struct ceph_mds_client *mdsc, struct dentry *dentry,

    if (pos < 0) {
            /*

•            * A rename didn't occur, but somehow we didn't end up where
  

•            * we thought we would. Throw a warning and try again.
  

•            * The path is longer than PATH_MAX and this function
  

•            * cannot ever succeed.  Creating paths that long is
  

•            * possible with Ceph, but Linux cannot use them.
             */
  

•           pr_warn_client(cl, "did not end path lookup where expected (pos = %d)\n",
  

•                          pos);
  

•           goto retry;
  

•           return ERR_PTR(-ENAMETOOLONG);
    }
  
    *pbase = base;
  

-- 2.45.2

I think the idea was that if we ended up with a path longer than PATH_MAX that something must have changed in the middle of the reverse pathwalk to make it too long a path, and we should just go and try it again. At the very least, this code should cap the retries to a certain number if you don't just return an error here.

-ENAMETOOLONG could be problematic there. This function is often called when we have a dentry and need to build a path to it to send to the MDS in a call. The system call that caused us to generate this path probably doesn't involve a pathname itself, so the caller may be confused by an -ENAMETOOLONG return.

You may want to go with a more generic error code here -- -EIO or something. It might also be worthwhile to leave in a pr_warn_once or something since there may be users confused by this error return.