[PATCH] attr: block mode changes of symlinks

[PATCH] attr: block mode changes of symlinks

[Christian Brauner]

Changing the mode of symlinks is meaningless as the vfs doesn't take the
mode of a symlink into account during path lookup permission checking.

However, the vfs doesn't block mode changes on symlinks. This however,
has lead to an untenable mess roughly classifiable into the following
two categories:

(1) Filesystems that don't implement a i_op->setattr() for symlinks.

    Such filesystems may or may not know that without i_op->setattr()
    defined, notify_change() falls back to simple_setattr() causing the
    inode's mode in the inode cache to be changed.

    That's a generic issue as this will affect all non-size changing
    inode attributes including ownership changes.

    Example: afs

(2) Filesystems that fail with EOPNOTSUPP but change the mode of the
    symlink nonetheless.

    Some filesystems will happily update the mode of a symlink but still
    return EOPNOTSUPP. This is the biggest source of confusion for
    userspace.

    The EOPNOTSUPP in this case comes from POSIX ACLs. Specifically it
    comes from filesystems that call posix_acl_chmod(), e.g., btrfs via

        if (!err && attr->ia_valid & ATTR_MODE)
                err = posix_acl_chmod(idmap, dentry, inode->i_mode);

    Filesystems including btrfs don't implement i_op->set_acl() so
    posix_acl_chmod() will report EOPNOTSUPP.

    When posix_acl_chmod() is called, most filesystems will have
    finished updating the inode.

    Perversely, this has the consequences that this behavior may depend
    on two kconfig options and mount options:

    * CONFIG_POSIX_ACL={y,n}
    * CONFIG_${FSTYPE}_POSIX_ACL={y,n}
    * Opt_acl, Opt_noacl

    Example: btrfs, ext4, xfs

The only way to change the mode on a symlink currently involves abusing
an O_PATH file descriptor in the following manner:

        fd = openat(-1, "/path/to/link", O_CLOEXEC | O_PATH | O_NOFOLLOW);

        char path[PATH_MAX];
        snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
        chmod(path, 0000);

But for most major filesystems with POSIX ACL support such as btrfs,
ext4, ceph, tmpfs, xfs and others this will fail with EOPNOTSUPP with
the mode still updated due to the aforementioned posix_acl_chmod()
nonsense.

So, given that for all major filesystems this would fail with EOPNOTSUPP
and that both glibc (cf. [1]) and musl (cf. [2]) outright block mode
changes on symlinks we should just try and block mode changes on
symlinks directly in the vfs and have a clean break with this nonsense.

If this causes any regressions, we do the next best thing and fix up all
filesystems that do return EOPNOTSUPP with the mode updated to not call
posix_acl_chmod() on symlinks.

But as usual, let's try the clean cut solution first. It's a simple
patch that can be easily reverted. Not marking this for backport as I'll
do that manually if we're reasonably sure that this works and there are
no strong objections.

We could block this in chmod_common() but it's more appropriate to do it
notify_change() as it will also mean that we catch filesystems that
change symlink permissions explicitly or accidently.

Similar proposals were floated in the past as in [3] and [4] and again
recently in [5]. There's also a couple of bugs about this inconsistency
as in [6] and [7].

Link: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=99527a3727e44cb8661ee1f743068f108ec93979;hb=HEAD [1]
Link: https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c [2]
Link: https://lore.kernel.org/all/20200911065733.GA31579@infradead.org [3]
Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00518.html [4]
Link: https://lore.kernel.org/lkml/87lefmbppo.fsf@oldenburg.str.redhat.com [5]
Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00467.html [6]
Link: https://sourceware.org/bugzilla/show_bug.cgi?id=14578#c17 [7]
Cc: stable@vger.kernel.org # no backport before v6.6-rc2 is tagged
Suggested-by: Christoph Hellwig <hch@lst.de>
Suggested-by: Florian Weimer <fweimer@redhat.com>
Reviewed-by: Aleksa Sarai <cyphar@cyphar.com>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---

---
 fs/attr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/attr.c b/fs/attr.c
index d60dc1edb526..529153ecde25 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -394,6 +394,9 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
 		return error;
 
 	if ((ia_valid & ATTR_MODE)) {
+		if (S_ISLNK(inode->i_mode))
+			return -EOPNOTSUPP;
+
 		umode_t amode = attr->ia_mode;
 		/* Flag setting protected by i_mutex */
 		if (is_sxid(amode))

---
base-commit: 06c2afb862f9da8dc5efa4b6076a0e48c3fbaaa5
change-id: 20230712-vfs-chmod-symlinks-a3ad1923a992

Re: [PATCH] attr: block mode changes of symlinks

[Greg KH]

On Wed, Jul 12, 2023 at 11:56:35AM +0200, Christian Brauner wrote:
> Changing the mode of symlinks is meaningless as the vfs doesn't take the
> mode of a symlink into account during path lookup permission checking.
> 
> However, the vfs doesn't block mode changes on symlinks. This however,
> has lead to an untenable mess roughly classifiable into the following
> two categories:
> 
> (1) Filesystems that don't implement a i_op->setattr() for symlinks.
> 
>     Such filesystems may or may not know that without i_op->setattr()
>     defined, notify_change() falls back to simple_setattr() causing the
>     inode's mode in the inode cache to be changed.
> 
>     That's a generic issue as this will affect all non-size changing
>     inode attributes including ownership changes.
> 
>     Example: afs
> 
> (2) Filesystems that fail with EOPNOTSUPP but change the mode of the
>     symlink nonetheless.
> 
>     Some filesystems will happily update the mode of a symlink but still
>     return EOPNOTSUPP. This is the biggest source of confusion for
>     userspace.
> 
>     The EOPNOTSUPP in this case comes from POSIX ACLs. Specifically it
>     comes from filesystems that call posix_acl_chmod(), e.g., btrfs via
> 
>         if (!err && attr->ia_valid & ATTR_MODE)
>                 err = posix_acl_chmod(idmap, dentry, inode->i_mode);
> 
>     Filesystems including btrfs don't implement i_op->set_acl() so
>     posix_acl_chmod() will report EOPNOTSUPP.
> 
>     When posix_acl_chmod() is called, most filesystems will have
>     finished updating the inode.
> 
>     Perversely, this has the consequences that this behavior may depend
>     on two kconfig options and mount options:
> 
>     * CONFIG_POSIX_ACL={y,n}
>     * CONFIG_${FSTYPE}_POSIX_ACL={y,n}
>     * Opt_acl, Opt_noacl
> 
>     Example: btrfs, ext4, xfs
> 
> The only way to change the mode on a symlink currently involves abusing
> an O_PATH file descriptor in the following manner:
> 
>         fd = openat(-1, "/path/to/link", O_CLOEXEC | O_PATH | O_NOFOLLOW);
> 
>         char path[PATH_MAX];
>         snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
>         chmod(path, 0000);
> 
> But for most major filesystems with POSIX ACL support such as btrfs,
> ext4, ceph, tmpfs, xfs and others this will fail with EOPNOTSUPP with
> the mode still updated due to the aforementioned posix_acl_chmod()
> nonsense.
> 
> So, given that for all major filesystems this would fail with EOPNOTSUPP
> and that both glibc (cf. [1]) and musl (cf. [2]) outright block mode
> changes on symlinks we should just try and block mode changes on
> symlinks directly in the vfs and have a clean break with this nonsense.
> 
> If this causes any regressions, we do the next best thing and fix up all
> filesystems that do return EOPNOTSUPP with the mode updated to not call
> posix_acl_chmod() on symlinks.
> 
> But as usual, let's try the clean cut solution first. It's a simple
> patch that can be easily reverted. Not marking this for backport as I'll
> do that manually if we're reasonably sure that this works and there are
> no strong objections.
> 
> We could block this in chmod_common() but it's more appropriate to do it
> notify_change() as it will also mean that we catch filesystems that
> change symlink permissions explicitly or accidently.
> 
> Similar proposals were floated in the past as in [3] and [4] and again
> recently in [5]. There's also a couple of bugs about this inconsistency
> as in [6] and [7].
> 
> Link: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=99527a3727e44cb8661ee1f743068f108ec93979;hb=HEAD [1]
> Link: https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c [2]
> Link: https://lore.kernel.org/all/20200911065733.GA31579@infradead.org [3]
> Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00518.html [4]
> Link: https://lore.kernel.org/lkml/87lefmbppo.fsf@oldenburg.str.redhat.com [5]
> Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00467.html [6]
> Link: https://sourceware.org/bugzilla/show_bug.cgi?id=14578#c17 [7]
> Cc: stable@vger.kernel.org # no backport before v6.6-rc2 is tagged

How far back should this go?

thanks,

greg k-h

Re: [PATCH] attr: block mode changes of symlinks

[Linus Torvalds]

On Wed, 12 Jul 2023 at 02:56, Christian Brauner <brauner@kernel.org> wrote:
>
> Changing the mode of symlinks is meaningless as the vfs doesn't take the
> mode of a symlink into account during path lookup permission checking.

Hmm. I have this dim memory that we actually used to do that as an
extension at one point for the symlinks in /proc. Long long ago.

Or maybe it was just a potential plan.

Because at least in /proc, the symlinks *do* have protection semantics
(ie you can't do readlink() on them or follow them without the right
permissions.

That said, blocking the mode setting sounds fine, because the proc
permissions are basically done separately.

However:

>         if ((ia_valid & ATTR_MODE)) {
> +               if (S_ISLNK(inode->i_mode))
> +                       return -EOPNOTSUPP;
> +
>                 umode_t amode = attr->ia_mode;

The above is not ok. It might compile these days because we have to
allow statements before declarations for other reasons, but that
doesn't make it ok.

             Linus

Re: [PATCH] attr: block mode changes of symlinks

[Christian Brauner]

On Wed, Jul 12, 2023 at 09:24:43AM -0700, Linus Torvalds wrote:
> On Wed, 12 Jul 2023 at 02:56, Christian Brauner <brauner@kernel.org> wrote:
> >
> > Changing the mode of symlinks is meaningless as the vfs doesn't take the
> > mode of a symlink into account during path lookup permission checking.
> 
> Hmm. I have this dim memory that we actually used to do that as an
> extension at one point for the symlinks in /proc. Long long ago.

If we block it properly now. We could - crazy talk on my side now:
through a sysctl like the weird sysctl sysctl_protected_* stuff we have
already - later implement taking the mode of symlinks into account
properly. I'm not saying we should nor that it's wise but it would be
doable.

> 
> Or maybe it was just a potential plan.
> 
> Because at least in /proc, the symlinks *do* have protection semantics
> (ie you can't do readlink() on them or follow them without the right
> permissions.
> 
> That said, blocking the mode setting sounds fine, because the proc
> permissions are basically done separately.
> 
> However:
> 
> >         if ((ia_valid & ATTR_MODE)) {
> > +               if (S_ISLNK(inode->i_mode))
> > +                       return -EOPNOTSUPP;
> > +
> >                 umode_t amode = attr->ia_mode;
> 
> The above is not ok. It might compile these days because we have to
> allow statements before declarations for other reasons, but that
> doesn't make it ok.

Sorry, I completely missed that. I miss the days when that would've
thrown a compile error right away. Let me send a v2 right now.

Re: [PATCH] attr: block mode changes of symlinks

[Christian Brauner]

On Wed, Jul 12, 2023 at 06:21:51PM +0200, Greg KH wrote:
> On Wed, Jul 12, 2023 at 11:56:35AM +0200, Christian Brauner wrote:
> > Changing the mode of symlinks is meaningless as the vfs doesn't take the
> > mode of a symlink into account during path lookup permission checking.
> > 
> > However, the vfs doesn't block mode changes on symlinks. This however,
> > has lead to an untenable mess roughly classifiable into the following
> > two categories:
> > 
> > (1) Filesystems that don't implement a i_op->setattr() for symlinks.
> > 
> >     Such filesystems may or may not know that without i_op->setattr()
> >     defined, notify_change() falls back to simple_setattr() causing the
> >     inode's mode in the inode cache to be changed.
> > 
> >     That's a generic issue as this will affect all non-size changing
> >     inode attributes including ownership changes.
> > 
> >     Example: afs
> > 
> > (2) Filesystems that fail with EOPNOTSUPP but change the mode of the
> >     symlink nonetheless.
> > 
> >     Some filesystems will happily update the mode of a symlink but still
> >     return EOPNOTSUPP. This is the biggest source of confusion for
> >     userspace.
> > 
> >     The EOPNOTSUPP in this case comes from POSIX ACLs. Specifically it
> >     comes from filesystems that call posix_acl_chmod(), e.g., btrfs via
> > 
> >         if (!err && attr->ia_valid & ATTR_MODE)
> >                 err = posix_acl_chmod(idmap, dentry, inode->i_mode);
> > 
> >     Filesystems including btrfs don't implement i_op->set_acl() so
> >     posix_acl_chmod() will report EOPNOTSUPP.
> > 
> >     When posix_acl_chmod() is called, most filesystems will have
> >     finished updating the inode.
> > 
> >     Perversely, this has the consequences that this behavior may depend
> >     on two kconfig options and mount options:
> > 
> >     * CONFIG_POSIX_ACL={y,n}
> >     * CONFIG_${FSTYPE}_POSIX_ACL={y,n}
> >     * Opt_acl, Opt_noacl
> > 
> >     Example: btrfs, ext4, xfs
> > 
> > The only way to change the mode on a symlink currently involves abusing
> > an O_PATH file descriptor in the following manner:
> > 
> >         fd = openat(-1, "/path/to/link", O_CLOEXEC | O_PATH | O_NOFOLLOW);
> > 
> >         char path[PATH_MAX];
> >         snprintf(path, sizeof(path), "/proc/self/fd/%d", fd);
> >         chmod(path, 0000);
> > 
> > But for most major filesystems with POSIX ACL support such as btrfs,
> > ext4, ceph, tmpfs, xfs and others this will fail with EOPNOTSUPP with
> > the mode still updated due to the aforementioned posix_acl_chmod()
> > nonsense.
> > 
> > So, given that for all major filesystems this would fail with EOPNOTSUPP
> > and that both glibc (cf. [1]) and musl (cf. [2]) outright block mode
> > changes on symlinks we should just try and block mode changes on
> > symlinks directly in the vfs and have a clean break with this nonsense.
> > 
> > If this causes any regressions, we do the next best thing and fix up all
> > filesystems that do return EOPNOTSUPP with the mode updated to not call
> > posix_acl_chmod() on symlinks.
> > 
> > But as usual, let's try the clean cut solution first. It's a simple
> > patch that can be easily reverted. Not marking this for backport as I'll
> > do that manually if we're reasonably sure that this works and there are
> > no strong objections.
> > 
> > We could block this in chmod_common() but it's more appropriate to do it
> > notify_change() as it will also mean that we catch filesystems that
> > change symlink permissions explicitly or accidently.
> > 
> > Similar proposals were floated in the past as in [3] and [4] and again
> > recently in [5]. There's also a couple of bugs about this inconsistency
> > as in [6] and [7].
> > 
> > Link: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/fchmodat.c;h=99527a3727e44cb8661ee1f743068f108ec93979;hb=HEAD [1]
> > Link: https://git.musl-libc.org/cgit/musl/tree/src/stat/fchmodat.c [2]
> > Link: https://lore.kernel.org/all/20200911065733.GA31579@infradead.org [3]
> > Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00518.html [4]
> > Link: https://lore.kernel.org/lkml/87lefmbppo.fsf@oldenburg.str.redhat.com [5]
> > Link: https://sourceware.org/legacy-ml/libc-alpha/2020-02/msg00467.html [6]
> > Link: https://sourceware.org/bugzilla/show_bug.cgi?id=14578#c17 [7]
> > Cc: stable@vger.kernel.org # no backport before v6.6-rc2 is tagged
> 
> How far back should this go?

If this holds up without regressions than all LTSes. That's what Amir
and Leah did for some other work. I can add that to the comment for
clarity.

Re: [PATCH] attr: block mode changes of symlinks

[Jesse Hathaway]

> If this holds up without regressions than all LTSes. That's what Amir
> and Leah did for some other work. I can add that to the comment for
> clarity.

Unfortunately, this has not held up in LTSes without causing
regressions, specifically in crun:

Crun issue and patch
 1. https://github.com/containers/crun/issues/1308
 2. https://github.com/containers/crun/pull/1309

Debian bug report
 1. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1053821

I think it should be reverted in LTSes and possibly in upstream.

Yours kindly, Jesse Hathaway

P.S. apologies for not having the correct threading headers. I am not on
the list.

Re: [PATCH] attr: block mode changes of symlinks

[Greg KH]

On Wed, Oct 18, 2023 at 01:34:13PM -0500, Jesse Hathaway wrote:
> > If this holds up without regressions than all LTSes. That's what Amir
> > and Leah did for some other work. I can add that to the comment for
> > clarity.
> 
> Unfortunately, this has not held up in LTSes without causing
> regressions, specifically in crun:
> 
> Crun issue and patch
>  1. https://github.com/containers/crun/issues/1308
>  2. https://github.com/containers/crun/pull/1309

So thre's a fix already for this, they agree that symlinks shouldn't
have modes, so what's the issue?

> Debian bug report
>  1. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1053821

Same report.

> I think it should be reverted in LTSes and possibly in upstream.

It needs to reverted in Linus's tree first, otherwise you will hit the
same problem when moving to a new kernel.

> P.S. apologies for not having the correct threading headers. I am not on
> the list.

You can always grab the mail on lore.kernel.org and respond to it there,
you are trying to dig up a months old email and we don't really have any
context at all (I had to go to lore to figure it out...)

thanks,

greg k-h

Re: [PATCH] attr: block mode changes of symlinks

[Jesse Hathaway]

On Wed, Oct 18, 2023 at 1:40 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Unfortunately, this has not held up in LTSes without causing
> > regressions, specifically in crun:
> >
> > Crun issue and patch
> >  1. https://github.com/containers/crun/issues/1308
> >  2. https://github.com/containers/crun/pull/1309
>
> So thre's a fix already for this, they agree that symlinks shouldn't
> have modes, so what's the issue?

The problem is that it breaks crun in Debian stable. They have fixed the
issue in crun, but that patch may not be backported to Debian's stable
version. In other words the patch seems to break existing software in
the wild.

> It needs to reverted in Linus's tree first, otherwise you will hit the
> same problem when moving to a new kernel.

Okay, I'll raise the issue on the linux kernel mailing list.

> > P.S. apologies for not having the correct threading headers. I am not on
> > the list.
>
> You can always grab the mail on lore.kernel.org and respond to it there,
> you are trying to dig up a months old email and we don't really have any
> context at all (I had to go to lore to figure it out...)

Thanks, I'll do that next time.

Re: [PATCH] attr: block mode changes of symlinks

[Greg KH]

On Wed, Oct 18, 2023 at 01:49:44PM -0500, Jesse Hathaway wrote:
> On Wed, Oct 18, 2023 at 1:40 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > Unfortunately, this has not held up in LTSes without causing
> > > regressions, specifically in crun:
> > >
> > > Crun issue and patch
> > >  1. https://github.com/containers/crun/issues/1308
> > >  2. https://github.com/containers/crun/pull/1309
> >
> > So thre's a fix already for this, they agree that symlinks shouldn't
> > have modes, so what's the issue?
> 
> The problem is that it breaks crun in Debian stable. They have fixed the
> issue in crun, but that patch may not be backported to Debian's stable
> version. In other words the patch seems to break existing software in
> the wild.

It will be backported to Debian stable if the kernel in Debian stable
has this change in it, right?  That should be simple to get accepted.

thanks,

greg k-h

Re: [PATCH] attr: block mode changes of symlinks

[Linux regression tracking (Thorsten Leemhuis)]

[adding Christian, the author of what appears to be the culprit]

On 18.10.23 20:49, Jesse Hathaway wrote:
> On Wed, Oct 18, 2023 at 1:40 PM Greg KH <gregkh@linuxfoundation.org> wrote:

FWIW, this thread afaics was supposed to be in reply to this submission:

https://lore.kernel.org/all/20230712-vfs-chmod-symlinks-v1-1-27921df6011f@kernel.org/

That patch later became 5d1f903f75a80d ("attr: block mode changes of
symlinks") [v6.6-rc1, v6.5.5, v6.1.55, v5.4.257, v5.15.133, v5.10.197,
v4.19.295, v4.14.326]

>>> Unfortunately, this has not held up in LTSes without causing
>>> regressions, specifically in crun:
>>>
>>> Crun issue and patch
>>>  1. https://github.com/containers/crun/issues/1308
>>>  2. https://github.com/containers/crun/pull/1309
>>
>> So thre's a fix already for this, they agree that symlinks shouldn't
>> have modes, so what's the issue?
> 
> The problem is that it breaks crun in Debian stable. They have fixed the
> issue in crun, but that patch may not be backported to Debian's stable
> version. In other words the patch seems to break existing software in
> the wild.
> 
>> It needs to reverted in Linus's tree first, otherwise you will hit the
>> same problem when moving to a new kernel.
> 
> Okay, I'll raise the issue on the linux kernel mailing list.

Did you do that? I could not find anything. Just wondering, as right now
there is still some time to fix this regression before 6.6 is released
(and then the fix can be backported to the stable trees, too).

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

Re: [PATCH] attr: block mode changes of symlinks

[Christian Brauner]

On Fri, Oct 20, 2023 at 10:34:36AM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
> [adding Christian, the author of what appears to be the culprit]
> 
> On 18.10.23 20:49, Jesse Hathaway wrote:
> > On Wed, Oct 18, 2023 at 1:40 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> FWIW, this thread afaics was supposed to be in reply to this submission:
> 
> https://lore.kernel.org/all/20230712-vfs-chmod-symlinks-v1-1-27921df6011f@kernel.org/
> 
> That patch later became 5d1f903f75a80d ("attr: block mode changes of
> symlinks") [v6.6-rc1, v6.5.5, v6.1.55, v5.4.257, v5.15.133, v5.10.197,
> v4.19.295, v4.14.326]
> 
> >>> Unfortunately, this has not held up in LTSes without causing
> >>> regressions, specifically in crun:
> >>>
> >>> Crun issue and patch
> >>>  1. https://github.com/containers/crun/issues/1308
> >>>  2. https://github.com/containers/crun/pull/1309
> >>
> >> So thre's a fix already for this, they agree that symlinks shouldn't
> >> have modes, so what's the issue?
> > 
> > The problem is that it breaks crun in Debian stable. They have fixed the
> > issue in crun, but that patch may not be backported to Debian's stable
> > version. In other words the patch seems to break existing software in
> > the wild.
> > 
> >> It needs to reverted in Linus's tree first, otherwise you will hit the
> >> same problem when moving to a new kernel.
> > 
> > Okay, I'll raise the issue on the linux kernel mailing list.
> 
> Did you do that? I could not find anything. Just wondering, as right now
> there is still some time to fix this regression before 6.6 is released
> (and then the fix can be backported to the stable trees, too).

I have not seen a report other than the crun fix I commented on.

The crun authors had agreed to fix this in crun. As symlink mode changes
are severly broken to the point that it's not even supported through the
official glibc and musl system call wrappers anymore not having to
revert this from mainline would be the ideal outcome.

So ideally, the crun bugfix would be backported to Debian stable just as
it was already backported to Fedora or crun make a new point release for
the 1.8.* series.

The other option to consider would be to revert the backport of the attr
changes to stable kernels. I'm not sure what Greg's stance on this is
but given that crun versions in -testing already include that fix that
means all future Debian releases will already have a fixed crun version.

That symlink stuff is so brittle and broken that we'd do more long-term
harm by letting it go on. Which is why we did this.

@Linus, this is ultimately your call of course.

Re: [PATCH] attr: block mode changes of symlinks

[Giuseppe Scrivano]

Christian Brauner <brauner@kernel.org> writes:

> On Fri, Oct 20, 2023 at 10:34:36AM +0200, Linux regression tracking (Thorsten Leemhuis) wrote:
>> [adding Christian, the author of what appears to be the culprit]
>> 
>> On 18.10.23 20:49, Jesse Hathaway wrote:
>> > On Wed, Oct 18, 2023 at 1:40 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>> 
>> FWIW, this thread afaics was supposed to be in reply to this submission:
>> 
>> https://lore.kernel.org/all/20230712-vfs-chmod-symlinks-v1-1-27921df6011f@kernel.org/
>> 
>> That patch later became 5d1f903f75a80d ("attr: block mode changes of
>> symlinks") [v6.6-rc1, v6.5.5, v6.1.55, v5.4.257, v5.15.133, v5.10.197,
>> v4.19.295, v4.14.326]
>> 
>> >>> Unfortunately, this has not held up in LTSes without causing
>> >>> regressions, specifically in crun:
>> >>>
>> >>> Crun issue and patch
>> >>>  1. https://github.com/containers/crun/issues/1308
>> >>>  2. https://github.com/containers/crun/pull/1309
>> >>
>> >> So thre's a fix already for this, they agree that symlinks shouldn't
>> >> have modes, so what's the issue?
>> > 
>> > The problem is that it breaks crun in Debian stable. They have fixed the
>> > issue in crun, but that patch may not be backported to Debian's stable
>> > version. In other words the patch seems to break existing software in
>> > the wild.
>> > 
>> >> It needs to reverted in Linus's tree first, otherwise you will hit the
>> >> same problem when moving to a new kernel.
>> > 
>> > Okay, I'll raise the issue on the linux kernel mailing list.
>> 
>> Did you do that? I could not find anything. Just wondering, as right now
>> there is still some time to fix this regression before 6.6 is released
>> (and then the fix can be backported to the stable trees, too).
>
> I have not seen a report other than the crun fix I commented on.
>
> The crun authors had agreed to fix this in crun. As symlink mode changes
> are severly broken to the point that it's not even supported through the
> official glibc and musl system call wrappers anymore not having to
> revert this from mainline would be the ideal outcome.
>
> So ideally, the crun bugfix would be backported to Debian stable just as
> it was already backported to Fedora or crun make a new point release for
> the 1.8.* series.
>
> The other option to consider would be to revert the backport of the attr
> changes to stable kernels. I'm not sure what Greg's stance on this is
> but given that crun versions in -testing already include that fix that
> means all future Debian releases will already have a fixed crun version.
>
> That symlink stuff is so brittle and broken that we'd do more long-term
> harm by letting it go on. Which is why we did this.
>
> @Linus, this is ultimately your call of course.

my two cents as the crun maintainer:

We were messing with /proc/*/fd files to do something not supported.
The kernel patch made the error explicit instead of ignoring errors just
in some cases.

Since it was already fixed upstream in crun and the fix is included in
the last three releases, Debian could simply pick a newer version; or I
can help with a backport if that is what they prefer.

Re: [PATCH] attr: block mode changes of symlinks

[Greg KH]

On Fri, Oct 20, 2023 at 01:01:44PM +0200, Christian Brauner wrote:
> The other option to consider would be to revert the backport of the attr
> changes to stable kernels. I'm not sure what Greg's stance on this is
> but given that crun versions in -testing already include that fix that
> means all future Debian releases will already have a fixed crun version.

I will be glad to revert a change in a stable tree that is also reverted
in Linus's tree, but to just "delay" a change getting into the tree,
that's not ok (either the change is good or not.)

thanks,

greg k-h