Bug 215687 - chown behavior on XFS is changed
Summary: chown behavior on XFS is changed
Status: NEW
Alias: None
Product: File System
Classification: Unclassified
Component: XFS (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: FileSystem/XFS Default Virtual Assignee
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-03-15 08:12 UTC by Zorro Lang
Modified: 2022-03-24 12:50 UTC (History)
0 users

See Also:
Kernel Version: xfs-5.18-merge-1
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description Zorro Lang 2022-03-15 08:12:27 UTC
From our regular regression test on XFS, we found a failure which never failed before:
# prove -rf /root/git/pjd-fstest/tests/chown/00.t 
/root/git/pjd-fstest/tests/chown/00.t .. 83/171 
not ok 84
not ok 88
/root/git/pjd-fstest/tests/chown/00.t .. Failed 2/171 subtests 

Test Summary Report
-------------------
/root/git/pjd-fstest/tests/chown/00.t (Wstat: 0 Tests: 171 Failed: 2)
  Failed tests:  84, 88
Files=1, Tests=171, 31 wallclock secs ( 0.10 usr  0.04 sys +  1.04 cusr 15.50 csys = 16.68 CPU)
Result: FAIL

That means chown behavior has been changed on XFS. To reproduce this failure you can:
1) mkfs.xfs -f /dev/sdb1
2) mount /dev/sdb1 /mnt/test
3) cd /mnt/test
4) git clone git://git.code.sf.net/p/ntfs-3g/pjd-fstest
5) cd pjd-fstest; make;
6) echo -e "os=Linux\nfs=xfs" > tests/conf
7) cd /mnt/test
8) prove -rf /path/to/pjd-fstest/tests/chown/00.t

I'm going to look into it, to found out what specified behavior is changed.
Comment 1 Zorro Lang 2022-03-15 08:30:44 UTC
Oh, looks like the failures come from below testing:

expect 0 mkdir ${n0} 0755
expect 0 chown ${n0} 65534 65533
expect 0 chmod ${n0} 06555
expect 06555 lstat ${n0} mode
expect 0 -u 65534 -g 65533,65532 chown ${n0} 65534 65532
case "${os}:${fs}" in
Linux:glusterfs)
        expect "0555,65534,65532|06555,65534,65532" lstat ${n0} mode,uid,gid
        ;;
Linux:xfs)
        expect 0555,65534,65532 lstat ${n0} mode,uid,gid
        ;;
Linux:*)
        expect 06555,65534,65532 lstat ${n0} mode,uid,gid
        ;;
*)
        expect 0555,65534,65532 lstat ${n0} mode,uid,gid
        ;;
esac

If a directory has S_ISUID(04000) and S_ISGID(02000) permission bits, XFS will lost these 2 bits after chown the group of the directory. But now it keeps these two bits:

# mkdir dir
# chown 65534:65533 dir
# chmod 06555 dir
# ls -ld dir
dr-sr-sr-x. 2 nobody 65533 6 Mar 15 16:26 dir
# chown 65534:65532 dir
# ls -ld dir
dr-sr-sr-x. 2 nobody 65532 6 Mar 15 16:26 dir


Hmm... please help to make sure if this's an expected behavior change, or an unexpected regression? Thanks.
Comment 2 Zorro Lang 2022-03-15 09:21:37 UTC
OK, by looking into xfs commits, I feel this patch brings in this behavior change:

commit e014f37db1a2d109afa750042ac4d69cf3e3d88e
Author: Darrick J. Wong <djwong@kernel.org>
Date:   Tue Mar 8 10:51:16 2022 -0800

    xfs: use setattr_copy to set vfs inode attributes

We treated it as an XFS behavior ~10 years, now it's changed ...
Comment 3 The Linux kernel's regression tracker (Thorsten Leemhuis) 2022-03-24 10:22:51 UTC
Hi, this is your Linux kernel regression tracker. Top-posting for once,
to make this easily accessible to everyone.

On 15.03.22 09:12, bugzilla-daemon@kernel.org wrote:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=215687
> 
>            Summary: chown behavior on XFS is changed

Darrick, what's up with this bug reported more than ten days ago? It's a
a regression reported the reporter even bisected to a change of yours
(e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") --
see the ticket for details) – but nothing happened afaics. Did the
discussion about this continue somewhere else or did it fall through the
cracks?

Anyway: I'm adding it to regzbot, my Linux kernel regression tracking bot:

#regzbot ^introduced e014f37db1a2d109afa750042ac4d69cf3e3d88e
#regzbot title xfs: chown behavior changed
#regzbot link: https://bugzilla.kernel.org/show_bug.cgi?id=215687
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)

P.S.: As the Linux kernel's regression tracker I'm getting a lot of
reports on my table. I can only look briefly into most of them and lack
knowledge about most of the areas they concern. I thus unfortunately
will sometimes get things wrong or miss something important. I hope
that's not the case here; if you think it is, don't hesitate to tell me
in a public reply, it's in everyone's interest to set the public record
straight.
Comment 4 Zorro Lang 2022-03-24 12:00:50 UTC
(In reply to The Linux kernel's regression tracker (Thorsten Leemhuis) from comment #3)
> Hi, this is your Linux kernel regression tracker. Top-posting for once,
> to make this easily accessible to everyone.
> 
> On 15.03.22 09:12, bugzilla-daemon@kernel.org wrote:
> >
> > https://bugzilla.kernel.org/show_bug.cgi?id=215687
> > 
> >            Summary: chown behavior on XFS is changed
> 
> Darrick, what's up with this bug reported more than ten days ago? It's a
> a regression reported the reporter even bisected to a change of yours
> (e014f37db1a2 ("xfs: use setattr_copy to set vfs inode attributes") --
> see the ticket for details) – but nothing happened afaics. Did the
> discussion about this continue somewhere else or did it fall through the
> cracks?
> 
> Anyway: I'm adding it to regzbot, my Linux kernel regression tracking bot:
> 
> #regzbot ^introduced e014f37db1a2d109afa750042ac4d69cf3e3d88e
> #regzbot title xfs: chown behavior changed
> #regzbot link: https://bugzilla.kernel.org/show_bug.cgi?id=215687
> #regzbot ignore-activity
> 
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> 
> P.S.: As the Linux kernel's regression tracker I'm getting a lot of
> reports on my table. I can only look briefly into most of them and lack
> knowledge about most of the areas they concern. I thus unfortunately
> will sometimes get things wrong or miss something important. I hope
> that's not the case here; if you think it is, don't hesitate to tell me
> in a public reply, it's in everyone's interest to set the public record
> straight.

Darrick has talked about it with us in IRC (as below, hope that helps):

2022-03-16 00:02 < djwong> all that setgid dropping came out of complaints that xfs didn't handle that the same way as all the other linux filesystems
2022-03-16 00:03 < djwong> zlang: ^^^
2022-03-16 00:04 < djwong> originally the xfs setattr more or less did what the vfs setattr did
2022-03-16 00:04 < djwong> but now people update the vfs setattr and they don't update the xfs version
2022-03-16 00:05 < djwong> so is this a "unique feature of xfs"?
2022-03-16 00:06 < djwong> inconsistent behavior from xfs?
2022-03-16 00:06 < djwong> or just bitrotting crap in the kernel?
2022-03-16 01:46 < zlang> djwong, sandeen: Thanks! I don't know if there's a standard describe that, just thought about how should we backport it, hope no customer depend on the old behavior :)
2022-03-16 01:55 < zlang> That would be great if no customer depend on that, or they might complain, if their script expect a program lose S_ISUID and S_ISGID after chown, but not, then cause permission/security problem
2022-03-16 02:00 < zlang> So if we backport that, we might be better to warn that in doc. To remind them if they hope to "lose" S_ISUID and S_ISGID bits, better to do that clearly and definitely
2022-03-16 02:01 < djwong> <nod> all that setgid handling is ... very murky
2022-03-16 02:01 < djwong> it at least matches ext4 and btrfs now :P


And another bug report (which can be closed DUP on this one):
https://bugzilla.kernel.org/show_bug.cgi?id=215693

Darrick has reviewed and replied in IRC (update as below):

2022-03-16 17:10 < zlang> djwong: Did you notice that generic/673 fails on xfs-5.18-merge-1, looks similar with that chown problem
2022-03-16 17:30 < zlang> https://bugzilla.kernel.org/show_bug.cgi?id=215693
2022-03-16 17:32 < zlang> But this's about reflink (not chown), and sometimes it lose sgid bit after reflink, sometimes not ...
2022-03-16 17:39 < zlang> So I report a seperate bug to track this question, please help to review and make sure the new expected behaviors. Sorry to bring this trouble to you
2022-03-17 00:36 < djwong> zlang: both setgid changes that you filed bugs against stem from the same setattr_copy issue
2022-03-17 00:37 < djwong> also generic/673 is wrong, see https://lore.kernel.org/fstests/164740142591.3371628.12793589713189041823.stgit@magnolia/T/#u
Comment 5 The Linux kernel's regression tracker (Thorsten Leemhuis) 2022-03-24 12:50:27 UTC
On 24.03.22 11:22, Thorsten Leemhuis wrote:
> On 15.03.22 09:12, bugzilla-daemon@kernel.org wrote:
>>
>> https://bugzilla.kernel.org/show_bug.cgi?id=215687
>>
>>            Summary: chown behavior on XFS is changed
> 
> Darrick, what's up with this bug reported more than ten days ago? 

Ahh, Zorro Lang replied in the ticket, seems things are okay now after
your discussed this on IRC, so let me remove this from the regression
tracking; if somebody really complains we ca start to track this again.

#regzbot invalid: seems reporter is okay with the changed behavior after
discussing this with the maintainer
https://bugzilla.kernel.org/show_bug.cgi?id=215687#c4

Note You need to log in before you can comment on or make changes to this bug.