public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
@ 2024-11-18 22:28 Max Kellermann
  2024-11-19 12:38 ` Ilya Dryomov
  0 siblings, 1 reply; 18+ messages in thread
From: Max Kellermann @ 2024-11-18 22:28 UTC (permalink / raw)
  To: xiubli, idryomov, ceph-devel, linux-kernel, dario; +Cc: stable, Max Kellermann

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.

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


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-18 22:28 [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX Max Kellermann
@ 2024-11-19 12:38 ` Ilya Dryomov
  2024-11-19 12:51   ` Jeff Layton
  0 siblings, 1 reply; 18+ messages in thread
From: Ilya Dryomov @ 2024-11-19 12:38 UTC (permalink / raw)
  To: Max Kellermann, Patrick Donnelly, Venky Shankar
  Cc: xiubli, ceph-devel, linux-kernel, dario, stable, Jeff Layton

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/?id=f5946bcc5e79038f9f7cb66ec25bd3b2d39b2775
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f6a9bc336b600e1266e6eebb0972d75d5b93aea9
[3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=991e72eb0e99764219865b9a3a07328695148e14

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
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 12:38 ` Ilya Dryomov
@ 2024-11-19 12:51   ` Jeff Layton
  2024-11-19 13:02     ` Max Kellermann
  0 siblings, 1 reply; 18+ messages in thread
From: Jeff Layton @ 2024-11-19 12:51 UTC (permalink / raw)
  To: Ilya Dryomov, Max Kellermann, Patrick Donnelly, Venky Shankar
  Cc: xiubli, ceph-devel, linux-kernel, dario, stable

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/?id=f5946bcc5e79038f9f7cb66ec25bd3b2d39b2775
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f6a9bc336b600e1266e6eebb0972d75d5b93aea9
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=991e72eb0e99764219865b9a3a07328695148e14
> 
> 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.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 12:51   ` Jeff Layton
@ 2024-11-19 13:02     ` Max Kellermann
  2024-11-19 13:32       ` Jeff Layton
  2024-11-19 13:58       ` Patrick Donnelly
  0 siblings, 2 replies; 18+ messages in thread
From: Max Kellermann @ 2024-11-19 13:02 UTC (permalink / raw)
  To: Jeff Layton
  Cc: Ilya Dryomov, Patrick Donnelly, Venky Shankar, xiubli, ceph-devel,
	linux-kernel, dario, stable

On Tue, Nov 19, 2024 at 1:51 PM Jeff Layton <jlayton@kernel.org> wrote:
> -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.

It is unfortunate that the Ceph-MDS protocol requires having to
convert a file descriptor back to a path name - but do you really
believe EIO would cause less confusion? ENAMETOOLONG is exactly what
happens, even if it's an internal error. But there are many error
codes that describe internal errors, so there's some prior art.

EIO just doesn't fit, returning EIO would be confusing - even more so
because EIO isn't a documented error code for open().

If this is about building path names for sending to the MDS, and not
for the userspace ABI, maybe the PATH_MAX limitation is wrong here. If
Ceph doesn't have such a limitation, the Ceph code shouldn't use the
userspace ABI limit for protocol use.

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

Users cannot read the kernel log, and this allows users to flood the
kernel log. So we get all the disadvantages of the kernel log while
our users get none of the advantages.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 13:02     ` Max Kellermann
@ 2024-11-19 13:32       ` Jeff Layton
  2024-11-19 13:58       ` Patrick Donnelly
  1 sibling, 0 replies; 18+ messages in thread
From: Jeff Layton @ 2024-11-19 13:32 UTC (permalink / raw)
  To: Max Kellermann
  Cc: Ilya Dryomov, Patrick Donnelly, Venky Shankar, xiubli, ceph-devel,
	linux-kernel, dario, stable

On Tue, 2024-11-19 at 14:02 +0100, Max Kellermann wrote:
> On Tue, Nov 19, 2024 at 1:51 PM Jeff Layton <jlayton@kernel.org> wrote:
> > -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.
> 
> It is unfortunate that the Ceph-MDS protocol requires having to
> convert a file descriptor back to a path name - but do you really
> believe EIO would cause less confusion? ENAMETOOLONG is exactly what
> happens, even if it's an internal error. But there are many error
> codes that describe internal errors, so there's some prior art.
> 
> EIO just doesn't fit, returning EIO would be confusing - even more so
> because EIO isn't a documented error code for open().
> 

Fair enough. EIO is just my goto when I don't have a better idea.

Probably you want some weird error code that will make people stand up
and take notice. Maybe?

     #define ENOSR           63      /* Out of streams resources */   

> If this is about building path names for sending to the MDS, and not
> for the userspace ABI, maybe the PATH_MAX limitation is wrong here. If
> Ceph doesn't have such a limitation, the Ceph code shouldn't use the
> userspace ABI limit for protocol use.
> 

It is possible to build a dir hierarchy that is deeper than can be
represented by a PATH_MAX-length string. No reason you can't allocate a
bigger buffer there. Does the MDS have a limit on the size of a path
string that it'll accept?

> > 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.
> 
> Users cannot read the kernel log, and this allows users to flood the
> kernel log. So we get all the disadvantages of the kernel log while
> our users get none of the advantages.

Sure. I'd make this a pr_warn_once or heavily ratelimit it or
something. No need for a lot of these messages, but eventually users
are going to wonder why they're getting these weird errors and bug the
admins. Giving them this hint might be helpful.

Alternately you could add a conditional tracepoint or something, but
those are more obscure.
-- 
Jeff Layton <jlayton@kernel.org>

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 13:02     ` Max Kellermann
  2024-11-19 13:32       ` Jeff Layton
@ 2024-11-19 13:58       ` Patrick Donnelly
  2024-11-19 14:54         ` Max Kellermann
  1 sibling, 1 reply; 18+ messages in thread
From: Patrick Donnelly @ 2024-11-19 13:58 UTC (permalink / raw)
  To: Max Kellermann
  Cc: Jeff Layton, Ilya Dryomov, Venky Shankar, xiubli, ceph-devel,
	linux-kernel, dario, stable

On Tue, Nov 19, 2024 at 8:02 AM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> On Tue, Nov 19, 2024 at 1:51 PM Jeff Layton <jlayton@kernel.org> wrote:
> > -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.
>
> It is unfortunate that the Ceph-MDS protocol requires having to
> convert a file descriptor back to a path name - but do you really
> believe EIO would cause less confusion? ENAMETOOLONG is exactly what
> happens, even if it's an internal error. But there are many error
> codes that describe internal errors, so there's some prior art.

The protocol does **not** require building the full path for most
operations unless it involves a snapshot. For snapshots we have to
climb the directory tree until we find the directory with the
snapshot. e.g.:

$ tree -a foo/ foo/.snap/ foo/bar/baz/.snap/
foo/
└── bar
    └── baz
        └── file
foo/.snap/
└── 1
    └── bar
        └── baz
            └── file
foo/bar/baz/.snap/
├── _1_1099511627779
│   └── file
└── 2
    └── file


If you read "file" via foo/.snap/1 you get:

2024-11-19T13:47:23.523+0000 7f9b3b79b640  1 --
[v2:172.21.10.4:6874/192645635,v1:172.21.10.4:6875/192645635] <==
client.4417 172.21.10.4:0/1322260999 43506 ====
client_request(client.4417:121 open #0x10000000003//1/bar/baz/file
2024-11-19T13:47:23.524518+0000 caller_uid=1141,
caller_gid=1141{1000,1141,}) ==== 199+0+0 (crc 0 0 0) 0x55acb2d55180
con 0x55acb2b3cc00

and for foo/bar/baz/.snap/2/

2024-11-19T13:47:56.796+0000 7f9b3b79b640  1 --
[v2:172.21.10.4:6874/192645635,v1:172.21.10.4:6875/192645635] <==
client.4417 172.21.10.4:0/1322260999 43578 ====
client_request(client.4417:155 open #0x10000000005//2/file
2024-11-19T13:47:56.798370+0000 caller_uid=1141,
caller_gid=1141{1000,1141,}) ==== 191+0+0 (crc 0 0 0) 0x55acb2d56000
con 0x55acb2b3cc00

(Note: the MDS protocol indicates a snapshot in the relative file path
via double forward slash.)

If you create "file":

2024-11-19T13:56:56.895+0000 7f9b34f8e640  7 mds.0.server
reply_client_request 0 ((0) Success) client_request(client.4467:7
create owner_uid=1141, owner_gid=1141 #0x10000000005/file
2024-11-19T13:56:56.890430+0000 caller_uid=1141,
caller_gid=1141{1000,1141,})

(During path lookups when planning to read the file, the client will
usually get read caps so it doesn't need to formally open the file. So
this last example uses a create.)


-- 
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 13:58       ` Patrick Donnelly
@ 2024-11-19 14:54         ` Max Kellermann
  2024-11-19 15:13           ` Patrick Donnelly
  0 siblings, 1 reply; 18+ messages in thread
From: Max Kellermann @ 2024-11-19 14:54 UTC (permalink / raw)
  To: Patrick Donnelly
  Cc: Jeff Layton, Ilya Dryomov, Venky Shankar, xiubli, ceph-devel,
	linux-kernel, dario, stable

On Tue, Nov 19, 2024 at 2:58 PM Patrick Donnelly <pdonnell@redhat.com> wrote:
> The protocol does **not** require building the full path for most
> operations unless it involves a snapshot.

We don't use Ceph snapshots, but before today's emergency update, we
could shoot down an arbitrary server with a single (unprivileged)
system call using this vulnerability.

I'm not sure what your point is, but this vulnerability exists, it
works without snapshots and we think it's serious.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 14:54         ` Max Kellermann
@ 2024-11-19 15:13           ` Patrick Donnelly
  2024-11-21 10:48             ` Alex Markuze
  0 siblings, 1 reply; 18+ messages in thread
From: Patrick Donnelly @ 2024-11-19 15:13 UTC (permalink / raw)
  To: Max Kellermann
  Cc: Jeff Layton, Ilya Dryomov, Venky Shankar, xiubli, ceph-devel,
	linux-kernel, dario, stable

On Tue, Nov 19, 2024 at 9:54 AM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> On Tue, Nov 19, 2024 at 2:58 PM Patrick Donnelly <pdonnell@redhat.com> wrote:
> > The protocol does **not** require building the full path for most
> > operations unless it involves a snapshot.
>
> We don't use Ceph snapshots, but before today's emergency update, we
> could shoot down an arbitrary server with a single (unprivileged)
> system call using this vulnerability.
>
> I'm not sure what your point is, but this vulnerability exists, it
> works without snapshots and we think it's serious.

I'm not suggesting there isn't a bug. I'm correcting a misunderstanding.

-- 
Patrick Donnelly, Ph.D.
He / Him / His
Red Hat Partner Engineer
IBM, Inc.
GPG: 19F28A586F808C2402351B93C3301A3E258DD79D


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-19 15:13           ` Patrick Donnelly
@ 2024-11-21 10:48             ` Alex Markuze
  2024-11-25 13:23               ` Alex Markuze
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Markuze @ 2024-11-21 10:48 UTC (permalink / raw)
  To: Patrick Donnelly
  Cc: Max Kellermann, Jeff Layton, Ilya Dryomov, Venky Shankar, xiubli,
	ceph-devel, linux-kernel, dario, stable

IMHO, we should first have a solution for the immediate problem,
remove infinite retries and fail early, and cap it at 3 retries in
case there is a temporary issue here.
I would use ENAMETOOLONG as the primary error code, as it is the most
informative, and couple it with a rate-limited kernel log
(pr_warn_once) for debugging without flooding.
I would also open a bug/feature request for a dynamic buffer
allocation that bypasses PATH_MAX for protocol-specific paths.

On Tue, Nov 19, 2024 at 5:17 PM Patrick Donnelly <pdonnell@redhat.com> wrote:
>
> On Tue, Nov 19, 2024 at 9:54 AM Max Kellermann <max.kellermann@ionos.com> wrote:
> >
> > On Tue, Nov 19, 2024 at 2:58 PM Patrick Donnelly <pdonnell@redhat.com> wrote:
> > > The protocol does **not** require building the full path for most
> > > operations unless it involves a snapshot.
> >
> > We don't use Ceph snapshots, but before today's emergency update, we
> > could shoot down an arbitrary server with a single (unprivileged)
> > system call using this vulnerability.
> >
> > I'm not sure what your point is, but this vulnerability exists, it
> > works without snapshots and we think it's serious.
>
> I'm not suggesting there isn't a bug. I'm correcting a misunderstanding.
>
> --
> Patrick Donnelly, Ph.D.
> He / Him / His
> Red Hat Partner Engineer
> IBM, Inc.
> GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
>
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-21 10:48             ` Alex Markuze
@ 2024-11-25 13:23               ` Alex Markuze
  2024-11-25 13:59                 ` Max Kellermann
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Markuze @ 2024-11-25 13:23 UTC (permalink / raw)
  To: Patrick Donnelly
  Cc: Max Kellermann, Jeff Layton, Ilya Dryomov, Venky Shankar, xiubli,
	ceph-devel, linux-kernel, dario, stable

Hey, Folks.
This seems important, so I'm bumping this thread. Max has a valid
concern, and this issue must be addressed.
Jeff seems to think keeping at least a few retries might be a good idea.

Max, could you add a cap on the retry count to your original patch? I
will discuss the PATH_MAX with Patrick and open a feature request for
myself to alleviate the limitation.

On Thu, Nov 21, 2024 at 12:48 PM Alex Markuze <amarkuze@redhat.com> wrote:
>
> IMHO, we should first have a solution for the immediate problem,
> remove infinite retries and fail early, and cap it at 3 retries in
> case there is a temporary issue here.
> I would use ENAMETOOLONG as the primary error code, as it is the most
> informative, and couple it with a rate-limited kernel log
> (pr_warn_once) for debugging without flooding.
> I would also open a bug/feature request for a dynamic buffer
> allocation that bypasses PATH_MAX for protocol-specific paths.
>
> On Tue, Nov 19, 2024 at 5:17 PM Patrick Donnelly <pdonnell@redhat.com> wrote:
> >
> > On Tue, Nov 19, 2024 at 9:54 AM Max Kellermann <max.kellermann@ionos.com> wrote:
> > >
> > > On Tue, Nov 19, 2024 at 2:58 PM Patrick Donnelly <pdonnell@redhat.com> wrote:
> > > > The protocol does **not** require building the full path for most
> > > > operations unless it involves a snapshot.
> > >
> > > We don't use Ceph snapshots, but before today's emergency update, we
> > > could shoot down an arbitrary server with a single (unprivileged)
> > > system call using this vulnerability.
> > >
> > > I'm not sure what your point is, but this vulnerability exists, it
> > > works without snapshots and we think it's serious.
> >
> > I'm not suggesting there isn't a bug. I'm correcting a misunderstanding.
> >
> > --
> > Patrick Donnelly, Ph.D.
> > He / Him / His
> > Red Hat Partner Engineer
> > IBM, Inc.
> > GPG: 19F28A586F808C2402351B93C3301A3E258DD79D
> >
> >


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-25 13:23               ` Alex Markuze
@ 2024-11-25 13:59                 ` Max Kellermann
  2024-11-25 14:32                   ` Alex Markuze
  0 siblings, 1 reply; 18+ messages in thread
From: Max Kellermann @ 2024-11-25 13:59 UTC (permalink / raw)
  To: Alex Markuze
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

On Mon, Nov 25, 2024 at 2:24 PM Alex Markuze <amarkuze@redhat.com> wrote:
> Max, could you add a cap on the retry count to your original patch?

Before I wrote code that's not useful at all: I don't quite get why
retry on buffer overflow is necessary at all. It looks like it once
seemed to be a useful kludge, but then 1b71fe2efa31 ("ceph analog of
cifs build_path_from_dentry() race fix") added the read_seqretry()
check which, to my limited understanding, is a more robust
implementation of rename detection.

Max

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-25 13:59                 ` Max Kellermann
@ 2024-11-25 14:32                   ` Alex Markuze
  2024-12-03 12:20                     ` Max Kellermann
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Markuze @ 2024-11-25 14:32 UTC (permalink / raw)
  To: Max Kellermann
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

You and Illia agree on this point. I'll wait for replies and take your
original patch into the testing branch unless any concerns are raised.

On Mon, Nov 25, 2024 at 3:59 PM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> On Mon, Nov 25, 2024 at 2:24 PM Alex Markuze <amarkuze@redhat.com> wrote:
> > Max, could you add a cap on the retry count to your original patch?
>
> Before I wrote code that's not useful at all: I don't quite get why
> retry on buffer overflow is necessary at all. It looks like it once
> seemed to be a useful kludge, but then 1b71fe2efa31 ("ceph analog of
> cifs build_path_from_dentry() race fix") added the read_seqretry()
> check which, to my limited understanding, is a more robust
> implementation of rename detection.
>
> Max
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-11-25 14:32                   ` Alex Markuze
@ 2024-12-03 12:20                     ` Max Kellermann
  2024-12-04 12:51                       ` Alex Markuze
  0 siblings, 1 reply; 18+ messages in thread
From: Max Kellermann @ 2024-12-03 12:20 UTC (permalink / raw)
  To: Alex Markuze
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

On Mon, Nov 25, 2024 at 3:33 PM Alex Markuze <amarkuze@redhat.com> wrote:
> You and Illia agree on this point. I'll wait for replies and take your
> original patch into the testing branch unless any concerns are raised.

How long will you wait? It's been more than two weeks since I reported
this DoS vulnerability.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-12-03 12:20                     ` Max Kellermann
@ 2024-12-04 12:51                       ` Alex Markuze
  2024-12-04 13:05                         ` Max Kellermann
  2024-12-05  8:24                         ` Max Kellermann
  0 siblings, 2 replies; 18+ messages in thread
From: Alex Markuze @ 2024-12-04 12:51 UTC (permalink / raw)
  To: Max Kellermann
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

It's already in a testing branch; what branch are you working on?

On Tue, Dec 3, 2024 at 2:21 PM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> On Mon, Nov 25, 2024 at 3:33 PM Alex Markuze <amarkuze@redhat.com> wrote:
> > You and Illia agree on this point. I'll wait for replies and take your
> > original patch into the testing branch unless any concerns are raised.
>
> How long will you wait? It's been more than two weeks since I reported
> this DoS vulnerability.
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-12-04 12:51                       ` Alex Markuze
@ 2024-12-04 13:05                         ` Max Kellermann
  2024-12-05  8:24                         ` Max Kellermann
  1 sibling, 0 replies; 18+ messages in thread
From: Max Kellermann @ 2024-12-04 13:05 UTC (permalink / raw)
  To: Alex Markuze
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

On Wed, Dec 4, 2024 at 1:51 PM Alex Markuze <amarkuze@redhat.com> wrote:
> It's already in a testing branch; what branch are you working on?

It is? Which one? I checked
https://github.com/ceph/ceph-client/commits/testing/ and it's not
there. This is the repository mentioned on
https://docs.ceph.com/en/latest/dev/developer_guide/testing_integration_tests/tests-integration-testing-teuthology-kernel/

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-12-04 12:51                       ` Alex Markuze
  2024-12-04 13:05                         ` Max Kellermann
@ 2024-12-05  8:24                         ` Max Kellermann
  2024-12-08 10:16                           ` Alex Markuze
  1 sibling, 1 reply; 18+ messages in thread
From: Max Kellermann @ 2024-12-05  8:24 UTC (permalink / raw)
  To: Alex Markuze
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

On Wed, Dec 4, 2024 at 1:51 PM Alex Markuze <amarkuze@redhat.com> wrote:
> It's already in a testing branch; what branch are you working on?

I found this on branch "wip-shirnk-crash":
https://github.com/ceph/ceph-client/commit/6cdec9f931e38980eb007d9704c5a24535fb5ec5
- did you mean this branch?

This is my patch; but you removed the commit message, removed the
explanation I wrote from the code comment, left the (useless and
confusing) log message in, and then claimed authorship for my work.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-12-05  8:24                         ` Max Kellermann
@ 2024-12-08 10:16                           ` Alex Markuze
  2024-12-10  8:42                             ` Ilya Dryomov
  0 siblings, 1 reply; 18+ messages in thread
From: Alex Markuze @ 2024-12-08 10:16 UTC (permalink / raw)
  To: Max Kellermann
  Cc: Patrick Donnelly, Jeff Layton, Ilya Dryomov, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

Illya, this patch is tested and it has my review by.

On Thu, Dec 5, 2024 at 10:24 AM Max Kellermann <max.kellermann@ionos.com> wrote:
>
> On Wed, Dec 4, 2024 at 1:51 PM Alex Markuze <amarkuze@redhat.com> wrote:
> > It's already in a testing branch; what branch are you working on?
>
> I found this on branch "wip-shirnk-crash":
> https://github.com/ceph/ceph-client/commit/6cdec9f931e38980eb007d9704c5a24535fb5ec5
> - did you mean this branch?
>
> This is my patch; but you removed the commit message, removed the
> explanation I wrote from the code comment, left the (useless and
> confusing) log message in, and then claimed authorship for my work.
>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX
  2024-12-08 10:16                           ` Alex Markuze
@ 2024-12-10  8:42                             ` Ilya Dryomov
  0 siblings, 0 replies; 18+ messages in thread
From: Ilya Dryomov @ 2024-12-10  8:42 UTC (permalink / raw)
  To: Alex Markuze
  Cc: Max Kellermann, Patrick Donnelly, Jeff Layton, Venky Shankar,
	xiubli, ceph-devel, linux-kernel, dario, stable

On Sun, Dec 8, 2024 at 11:17 AM Alex Markuze <amarkuze@redhat.com> wrote:
>
> Illya, this patch is tested and it has my review by.

Max's original patch has been applied (with the authorship, commit
message, etc preserved).

Thanks,

                Ilya

>
> On Thu, Dec 5, 2024 at 10:24 AM Max Kellermann <max.kellermann@ionos.com> wrote:
> >
> > On Wed, Dec 4, 2024 at 1:51 PM Alex Markuze <amarkuze@redhat.com> wrote:
> > > It's already in a testing branch; what branch are you working on?
> >
> > I found this on branch "wip-shirnk-crash":
> > https://github.com/ceph/ceph-client/commit/6cdec9f931e38980eb007d9704c5a24535fb5ec5
> > - did you mean this branch?
> >
> > This is my patch; but you removed the commit message, removed the
> > explanation I wrote from the code comment, left the (useless and
> > confusing) log message in, and then claimed authorship for my work.
> >
>

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2024-12-10  8:42 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-11-18 22:28 [PATCH] fs/ceph/mds_client: give up on paths longer than PATH_MAX Max Kellermann
2024-11-19 12:38 ` Ilya Dryomov
2024-11-19 12:51   ` Jeff Layton
2024-11-19 13:02     ` Max Kellermann
2024-11-19 13:32       ` Jeff Layton
2024-11-19 13:58       ` Patrick Donnelly
2024-11-19 14:54         ` Max Kellermann
2024-11-19 15:13           ` Patrick Donnelly
2024-11-21 10:48             ` Alex Markuze
2024-11-25 13:23               ` Alex Markuze
2024-11-25 13:59                 ` Max Kellermann
2024-11-25 14:32                   ` Alex Markuze
2024-12-03 12:20                     ` Max Kellermann
2024-12-04 12:51                       ` Alex Markuze
2024-12-04 13:05                         ` Max Kellermann
2024-12-05  8:24                         ` Max Kellermann
2024-12-08 10:16                           ` Alex Markuze
2024-12-10  8:42                             ` Ilya Dryomov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox