| Message ID | 20240604-unlink09-v1-1-dfd8e3e1cb2b@suse.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix unlink09 test | expand |
Hi Andrea, > From: Andrea Cervesato <andrea.cervesato@suse.com> > This patch will fix unlink09 test by checking for filesystems which > are not supporting inode attributes. > Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS) > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > This will fix the 2cf78f47a6 and resolve issues on filesystems > which are not supporting inode attributes. > --- > testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 13 deletions(-) > diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c > index cc4b4a07e..ed6f0adc3 100644 > --- a/testcases/kernel/syscalls/unlink/unlink09.c > +++ b/testcases/kernel/syscalls/unlink/unlink09.c > @@ -11,6 +11,8 @@ > * > * - EPERM when target file is marked as immutable or append-only > * - EROFS when target file is on a read-only filesystem. > + * > + * Test won't be executed if inode attributes are not supported. While this is good, wouldn't be better to use approach from Avinesh to add O_RDWR (or whatever flag) to get test supported everywhere instead of skip? https://patchwork.ozlabs.org/project/ltp/patch/20240603124653.31967-1-akumar@suse.de/ > */ > #include <sys/ioctl.h> > @@ -22,8 +24,8 @@ > #define DIR_EROFS "erofs" > #define TEST_EROFS "erofs/test_erofs" > -static int fd_immutable; > -static int fd_append_only; > +static int fd_immutable = -1; > +static int fd_append_only = -1; > static struct test_case_t { > char *filename; > @@ -43,12 +45,18 @@ static void setup(void) > { > int attr; > - fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600); > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); > + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); > + > + if (TST_RET == -1 && TST_ERR == ENOTTY) { > + SAFE_CLOSE(fd_immutable); > + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); > + } > + > attr |= FS_IMMUTABLE_FL; > SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > - fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600); > + fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600); > SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > attr |= FS_APPEND_FL; > SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > @@ -58,15 +66,19 @@ static void cleanup(void) > { > int attr; > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > - attr &= ~FS_IMMUTABLE_FL; > - SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > - SAFE_CLOSE(fd_immutable); > + if (fd_immutable != -1) { I thought we could return when fd_immutable == -1: if (fd_immutable != -1) return; But obviously this is can be also result of the first test (not only by the setup), thus you're correct. BTW verify_unlink() could be made simpler to return on if (TST_RET). > + SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > + attr &= ~FS_IMMUTABLE_FL; > + SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > + SAFE_CLOSE(fd_immutable); > + } > - SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > - attr &= ~FS_APPEND_FL; > - SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > - SAFE_CLOSE(fd_append_only); > + if (fd_append_only != -1) { > + SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > + attr &= ~FS_APPEND_FL; > + SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > + SAFE_CLOSE(fd_append_only); > + } Am I mistaken that this check should have been added before? > } > static void verify_unlink(unsigned int i) > --- > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f I see useful b4 feature :). > change-id: 20240604-unlink09-dc4802f872f9 But I wonder what is this for (what tool use change-id). Kind regards, Petr
Hi all, [ Cc the test author more experienced maintainers ] > Hi Andrea, > > From: Andrea Cervesato <andrea.cervesato@suse.com> > > This patch will fix unlink09 test by checking for filesystems which > > are not supporting inode attributes. > > Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS) > > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > > --- > > This will fix the 2cf78f47a6 and resolve issues on filesystems > > which are not supporting inode attributes. > > --- > > testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++---------- > > 1 file changed, 25 insertions(+), 13 deletions(-) > > diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c > > index cc4b4a07e..ed6f0adc3 100644 > > --- a/testcases/kernel/syscalls/unlink/unlink09.c > > +++ b/testcases/kernel/syscalls/unlink/unlink09.c > > @@ -11,6 +11,8 @@ > > * > > * - EPERM when target file is marked as immutable or append-only > > * - EROFS when target file is on a read-only filesystem. > > + * > > + * Test won't be executed if inode attributes are not supported. > While this is good, wouldn't be better to use approach from Avinesh to add > O_RDWR (or whatever flag) to get test supported everywhere instead of skip? > https://patchwork.ozlabs.org/project/ltp/patch/20240603124653.31967-1-akumar@suse.de/ OK, I got hint from Andrea, that this is inspired by statx04.c:86, which is filtering vfat and exfat. Here the problem was on system which has tmpfs, but just more strict default setup (likely umask). Therefore I still think that approach from Avinesh is correct. BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or ext4_unlink(), which are used for struct inode_operations unlink member. Then, obviously also Andrea's check would be needed (otherwise is unlikely that somebody would have TMPDIR on vfat or exfat). Kind regards, Petr > > */ > > #include <sys/ioctl.h> > > @@ -22,8 +24,8 @@ > > #define DIR_EROFS "erofs" > > #define TEST_EROFS "erofs/test_erofs" > > -static int fd_immutable; > > -static int fd_append_only; > > +static int fd_immutable = -1; > > +static int fd_append_only = -1; > > static struct test_case_t { > > char *filename; > > @@ -43,12 +45,18 @@ static void setup(void) > > { > > int attr; > > - fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600); > > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > > + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); > > + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); > > + > > + if (TST_RET == -1 && TST_ERR == ENOTTY) { > > + SAFE_CLOSE(fd_immutable); > > + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); > > + } > > + > > attr |= FS_IMMUTABLE_FL; > > SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > > - fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600); > > + fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600); > > SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > > attr |= FS_APPEND_FL; > > SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > > @@ -58,15 +66,19 @@ static void cleanup(void) > > { > > int attr; > > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > > - attr &= ~FS_IMMUTABLE_FL; > > - SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > > - SAFE_CLOSE(fd_immutable); > > + if (fd_immutable != -1) { > I thought we could return when fd_immutable == -1: > if (fd_immutable != -1) > return; > But obviously this is can be also result of the first test (not only by the > setup), thus you're correct. > BTW verify_unlink() could be made simpler to return on if (TST_RET). > > + SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > > + attr &= ~FS_IMMUTABLE_FL; > > + SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > > + SAFE_CLOSE(fd_immutable); > > + } > > - SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > > - attr &= ~FS_APPEND_FL; > > - SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > > - SAFE_CLOSE(fd_append_only); > > + if (fd_append_only != -1) { > > + SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > > + attr &= ~FS_APPEND_FL; > > + SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > > + SAFE_CLOSE(fd_append_only); > > + } > Am I mistaken that this check should have been added before? > > } > > static void verify_unlink(unsigned int i) > > --- > > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f > I see useful b4 feature :). > > change-id: 20240604-unlink09-dc4802f872f9 > But I wonder what is this for (what tool use change-id). > Kind regards, > Petr
Hi, On 6/5/24 09:38, Petr Vorel wrote: > Hi all, > > [ Cc the test author more experienced maintainers ] > >> Hi Andrea, >>> From: Andrea Cervesato <andrea.cervesato@suse.com> >>> This patch will fix unlink09 test by checking for filesystems which >>> are not supporting inode attributes. >>> Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS) >>> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> >>> --- >>> This will fix the 2cf78f47a6 and resolve issues on filesystems >>> which are not supporting inode attributes. >>> --- >>> testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++---------- >>> 1 file changed, 25 insertions(+), 13 deletions(-) >>> diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c >>> index cc4b4a07e..ed6f0adc3 100644 >>> --- a/testcases/kernel/syscalls/unlink/unlink09.c >>> +++ b/testcases/kernel/syscalls/unlink/unlink09.c >>> @@ -11,6 +11,8 @@ >>> * >>> * - EPERM when target file is marked as immutable or append-only >>> * - EROFS when target file is on a read-only filesystem. >>> + * >>> + * Test won't be executed if inode attributes are not supported. >> While this is good, wouldn't be better to use approach from Avinesh to add >> O_RDWR (or whatever flag) to get test supported everywhere instead of skip? >> https://patchwork.ozlabs.org/project/ltp/patch/20240603124653.31967-1-akumar@suse.de/ > OK, I got hint from Andrea, that this is inspired by statx04.c:86, which is > filtering vfat and exfat. Here the problem was on system which has tmpfs, but > just more strict default setup (likely umask). Therefore I still think that > approach from Avinesh is correct. statx04.c:86 is an example of what LTP tests do: they check if a feature is available or not in the specific environment. In our case, we need to skip filesystems which are not supporting inode attributes, due to the usage of ioctl(FS_IOC_GETFLAGS). And this is what unlink09 does in my patch. The Avinesh approach only changes flags for open(), which is still ok, but not enough and probably SAFE_CREAT() is better in our case. But if FS doesn't support inode attributes, test fails generating noise that requires debugging on the system as well, just to understand if there's a bug or not. And this is (of course) something that we would like to avoid... > BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS > only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or > ext4_unlink(), which are used for struct inode_operations unlink member. > Then, obviously also Andrea's check would be needed (otherwise is unlikely that > somebody would have TMPDIR on vfat or exfat). > > Kind regards, > Petr > >>> */ >>> #include <sys/ioctl.h> >>> @@ -22,8 +24,8 @@ >>> #define DIR_EROFS "erofs" >>> #define TEST_EROFS "erofs/test_erofs" >>> -static int fd_immutable; >>> -static int fd_append_only; >>> +static int fd_immutable = -1; >>> +static int fd_append_only = -1; >>> static struct test_case_t { >>> char *filename; >>> @@ -43,12 +45,18 @@ static void setup(void) >>> { >>> int attr; >>> - fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600); >>> - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); >>> + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); >>> + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); >>> + >>> + if (TST_RET == -1 && TST_ERR == ENOTTY) { >>> + SAFE_CLOSE(fd_immutable); >>> + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); >>> + } >>> + >>> attr |= FS_IMMUTABLE_FL; >>> SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); >>> - fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600); >>> + fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600); >>> SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); >>> attr |= FS_APPEND_FL; >>> SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); >>> @@ -58,15 +66,19 @@ static void cleanup(void) >>> { >>> int attr; >>> - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); >>> - attr &= ~FS_IMMUTABLE_FL; >>> - SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); >>> - SAFE_CLOSE(fd_immutable); >>> + if (fd_immutable != -1) { >> I thought we could return when fd_immutable == -1: >> if (fd_immutable != -1) >> return; >> But obviously this is can be also result of the first test (not only by the >> setup), thus you're correct. >> BTW verify_unlink() could be made simpler to return on if (TST_RET). >>> + SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); >>> + attr &= ~FS_IMMUTABLE_FL; >>> + SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); >>> + SAFE_CLOSE(fd_immutable); >>> + } >>> - SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); >>> - attr &= ~FS_APPEND_FL; >>> - SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); >>> - SAFE_CLOSE(fd_append_only); >>> + if (fd_append_only != -1) { >>> + SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); >>> + attr &= ~FS_APPEND_FL; >>> + SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); >>> + SAFE_CLOSE(fd_append_only); >>> + } >> Am I mistaken that this check should have been added before? >>> } >>> static void verify_unlink(unsigned int i) >>> --- >>> base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f >> I see useful b4 feature :). >>> change-id: 20240604-unlink09-dc4802f872f9 >> But I wonder what is this for (what tool use change-id). >> Kind regards, >> Petr Andrea
Hi! > > https://patchwork.ozlabs.org/project/ltp/patch/20240603124653.31967-1-akumar@suse.de/ > > OK, I got hint from Andrea, that this is inspired by statx04.c:86, which is > filtering vfat and exfat. Here the problem was on system which has tmpfs, but > just more strict default setup (likely umask). Therefore I still think that > approach from Avinesh is correct. > > BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS > only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or > ext4_unlink(), which are used for struct inode_operations unlink member. > Then, obviously also Andrea's check would be needed (otherwise is unlikely that > somebody would have TMPDIR on vfat or exfat). I would say that exfat or vfat is not uncommon in android world so I would expect that actually having LTP TMPDIR on exfat is not as unlikely as you may think.
Hi! > + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); > + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); > + > + if (TST_RET == -1 && TST_ERR == ENOTTY) { > + SAFE_CLOSE(fd_immutable); > + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); > + } I see one problem with this approach. If kernel accidentally removes a support for immutable files for a certain filesystem this test will be green. And the filesystems that miss this support are very unlikely to gain it, e.g. will vfat get support for immutable files? That would be an argument for explicit skiplist in the form of tst_test->skip_filesystems.
Hi, On 6/5/24 10:11, Cyril Hrubis wrote: > Hi! >> + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); >> + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); >> + >> + if (TST_RET == -1 && TST_ERR == ENOTTY) { >> + SAFE_CLOSE(fd_immutable); >> + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); >> + } > I see one problem with this approach. If kernel accidentally removes a > support for immutable files for a certain filesystem this test will be > green. And the filesystems that miss this support are very unlikely to > gain it, e.g. will vfat get support for immutable files? That would be > an argument for explicit skiplist in the form of > tst_test->skip_filesystems. > That's a valid statement. For now I would like to fix the test first, then we can fix this other problem with an another patch. Andrea
Hi! > >> + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); > >> + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); > >> + > >> + if (TST_RET == -1 && TST_ERR == ENOTTY) { > >> + SAFE_CLOSE(fd_immutable); > >> + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); > >> + } > > I see one problem with this approach. If kernel accidentally removes a > > support for immutable files for a certain filesystem this test will be > > green. And the filesystems that miss this support are very unlikely to > > gain it, e.g. will vfat get support for immutable files? That would be > > an argument for explicit skiplist in the form of > > tst_test->skip_filesystems. > > > That's a valid statement. For now I would like to fix the test first, > then we can fix this other problem with an another patch. As long as you promise to fix the test properly later on I agree with adding the temporary workaround with a test for immutable support. Also I suppose that it would make sense to enable the test for all_filesystems but we would have to move the EROFS to a separate test first.
Hi, On 6/5/24 13:30, Cyril Hrubis wrote: > Hi! >>>> + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); >>>> + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); >>>> + >>>> + if (TST_RET == -1 && TST_ERR == ENOTTY) { >>>> + SAFE_CLOSE(fd_immutable); >>>> + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); >>>> + } >>> I see one problem with this approach. If kernel accidentally removes a >>> support for immutable files for a certain filesystem this test will be >>> green. And the filesystems that miss this support are very unlikely to >>> gain it, e.g. will vfat get support for immutable files? That would be >>> an argument for explicit skiplist in the form of >>> tst_test->skip_filesystems. >>> >> That's a valid statement. For now I would like to fix the test first, >> then we can fix this other problem with an another patch. > As long as you promise to fix the test properly later on I agree with > adding the temporary workaround with a test for immutable support. The most important thing is that we can fix test so it won't show false negative on certain FS, then we can create patches to split into 2 tests (one for immutable and one for EROFS), using .all_filesystems flag. > > Also I suppose that it would make sense to enable the test for > all_filesystems but we would have to move the EROFS to a separate test > first. > Andrea
On 05. 06. 24 10:11, Cyril Hrubis wrote: > Hi! >> + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); >> + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); >> + >> + if (TST_RET == -1 && TST_ERR == ENOTTY) { >> + SAFE_CLOSE(fd_immutable); >> + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); >> + } > > I see one problem with this approach. If kernel accidentally removes a > support for immutable files for a certain filesystem this test will be > green. And the filesystems that miss this support are very unlikely to > gain it, e.g. will vfat get support for immutable files? That would be > an argument for explicit skiplist in the form of > tst_test->skip_filesystems. This condition doesn't check support of only the immutable flag, though. It checks that the filesystem supports querying and setting inode flags in general. We should add a new test for ioctl(FS_IOC_FLAGS) into the ioctl test directory, this is not the appropriate place for it.
On 05. 06. 24 9:38, Petr Vorel wrote: > BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS > only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or > ext4_unlink(), which are used for struct inode_operations unlink member. > Then, obviously also Andrea's check would be needed (otherwise is unlikely that > somebody would have TMPDIR on vfat or exfat). AFAICT, .all_filesystems and .needs_rofs are mutually exclusive at the moment.
Hi, I'm not sure whether removing the FS_APPEND_FL flag in cleanup() is actually needed but it's a question for another patch. Reviewed-by: Martin Doucha <mdoucha@suse.cz> On 04. 06. 24 15:44, Andrea Cervesato wrote: > From: Andrea Cervesato <andrea.cervesato@suse.com> > > This patch will fix unlink09 test by checking for filesystems which > are not supporting inode attributes. > > Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS) > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > This will fix the 2cf78f47a6 and resolve issues on filesystems > which are not supporting inode attributes. > --- > testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 13 deletions(-) > > diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c > index cc4b4a07e..ed6f0adc3 100644 > --- a/testcases/kernel/syscalls/unlink/unlink09.c > +++ b/testcases/kernel/syscalls/unlink/unlink09.c > @@ -11,6 +11,8 @@ > * > * - EPERM when target file is marked as immutable or append-only > * - EROFS when target file is on a read-only filesystem. > + * > + * Test won't be executed if inode attributes are not supported. > */ > > #include <sys/ioctl.h> > @@ -22,8 +24,8 @@ > #define DIR_EROFS "erofs" > #define TEST_EROFS "erofs/test_erofs" > > -static int fd_immutable; > -static int fd_append_only; > +static int fd_immutable = -1; > +static int fd_append_only = -1; > > static struct test_case_t { > char *filename; > @@ -43,12 +45,18 @@ static void setup(void) > { > int attr; > > - fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600); > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); > + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); > + > + if (TST_RET == -1 && TST_ERR == ENOTTY) { > + SAFE_CLOSE(fd_immutable); > + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); > + } > + > attr |= FS_IMMUTABLE_FL; > SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > > - fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600); > + fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600); > SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > attr |= FS_APPEND_FL; > SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > @@ -58,15 +66,19 @@ static void cleanup(void) > { > int attr; > > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > - attr &= ~FS_IMMUTABLE_FL; > - SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > - SAFE_CLOSE(fd_immutable); > + if (fd_immutable != -1) { > + SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > + attr &= ~FS_IMMUTABLE_FL; > + SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > + SAFE_CLOSE(fd_immutable); > + } > > - SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > - attr &= ~FS_APPEND_FL; > - SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > - SAFE_CLOSE(fd_append_only); > + if (fd_append_only != -1) { > + SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > + attr &= ~FS_APPEND_FL; > + SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > + SAFE_CLOSE(fd_append_only); > + } > } > > static void verify_unlink(unsigned int i) > > --- > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f > change-id: 20240604-unlink09-dc4802f872f9 > > Best regards,
Hi Martin, > On 05. 06. 24 9:38, Petr Vorel wrote: > > BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS > > only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or > > ext4_unlink(), which are used for struct inode_operations unlink member. > > Then, obviously also Andrea's check would be needed (otherwise is unlikely that > > somebody would have TMPDIR on vfat or exfat). > AFAICT, .all_filesystems and .needs_rofs are mutually exclusive at the > moment. Good point, I completely overlook .needs_rofs. That makes things clearer. ATM we have 3 other tests in syscalls/unlink. Not sure if all are filesystem specific (I would say yes, but not sure), but at least unlink05.c (tests deleting with unlink()) should be tested .all_filesystems. unlink07.c and unlink08.c test errno. Kind regards, Petr
Hi, actually, there's one more invalid SAFE_OPEN() in verify_unlink() that also needs to be fixed by this patch. It'd be best to move the SAFE_CREATE()+ioctl() to a special function that'll take just a filename and the inode flag that needs to be set. Please send a v2. On 04. 06. 24 15:44, Andrea Cervesato wrote: > From: Andrea Cervesato <andrea.cervesato@suse.com> > > This patch will fix unlink09 test by checking for filesystems which > are not supporting inode attributes. > > Fixes: 2cf78f47a6 (unlink: Add error tests for EPERM and EROFS) > Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com> > --- > This will fix the 2cf78f47a6 and resolve issues on filesystems > which are not supporting inode attributes. > --- > testcases/kernel/syscalls/unlink/unlink09.c | 38 +++++++++++++++++++---------- > 1 file changed, 25 insertions(+), 13 deletions(-) > > diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c > index cc4b4a07e..ed6f0adc3 100644 > --- a/testcases/kernel/syscalls/unlink/unlink09.c > +++ b/testcases/kernel/syscalls/unlink/unlink09.c > @@ -11,6 +11,8 @@ > * > * - EPERM when target file is marked as immutable or append-only > * - EROFS when target file is on a read-only filesystem. > + * > + * Test won't be executed if inode attributes are not supported. > */ > > #include <sys/ioctl.h> > @@ -22,8 +24,8 @@ > #define DIR_EROFS "erofs" > #define TEST_EROFS "erofs/test_erofs" > > -static int fd_immutable; > -static int fd_append_only; > +static int fd_immutable = -1; > +static int fd_append_only = -1; > > static struct test_case_t { > char *filename; > @@ -43,12 +45,18 @@ static void setup(void) > { > int attr; > > - fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600); > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); > + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); > + > + if (TST_RET == -1 && TST_ERR == ENOTTY) { > + SAFE_CLOSE(fd_immutable); > + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); > + } > + > attr |= FS_IMMUTABLE_FL; > SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > > - fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600); > + fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600); > SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > attr |= FS_APPEND_FL; > SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > @@ -58,15 +66,19 @@ static void cleanup(void) > { > int attr; > > - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > - attr &= ~FS_IMMUTABLE_FL; > - SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > - SAFE_CLOSE(fd_immutable); > + if (fd_immutable != -1) { > + SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); > + attr &= ~FS_IMMUTABLE_FL; > + SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); > + SAFE_CLOSE(fd_immutable); > + } > > - SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > - attr &= ~FS_APPEND_FL; > - SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > - SAFE_CLOSE(fd_append_only); > + if (fd_append_only != -1) { > + SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); > + attr &= ~FS_APPEND_FL; > + SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); > + SAFE_CLOSE(fd_append_only); > + } > } > > static void verify_unlink(unsigned int i) > > --- > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f > change-id: 20240604-unlink09-dc4802f872f9 > > Best regards,
Hi Martin, > Hi Martin, > > On 05. 06. 24 9:38, Petr Vorel wrote: > > > BTW shouldn't this test use .all_filesystems = 1 ? Or is it unlink() really VFS > > > only code? I see some specific functions in fs/*/, e.g. btrfs_unlink() or > > > ext4_unlink(), which are used for struct inode_operations unlink member. > > > Then, obviously also Andrea's check would be needed (otherwise is unlikely that > > > somebody would have TMPDIR on vfat or exfat). > > AFAICT, .all_filesystems and .needs_rofs are mutually exclusive at the > > moment. Also I wonder if having functionality for .all_filesystems + .needs_rofs wouldn't be useful. @Cyril @Martin WDYT? Also, there is fallback when prepare_and_mount_ro_fs() fails to use block device. Although, I don't see the read only mount flags added in this fallback, IMHO MS_RDONLY is only in prepare_and_mount_ro_fs(), therefore the fallback is read write and we even didn't get TWARN, just plain TINFO (it should be either TWARN or TINFO with "WARNING:" at least). Kind regards, Petr lib/tst_test.c static void prepare_device(void) { ... if (tst_test->needs_rofs) { /* If we failed to mount read-only tmpfs. Fallback to * using a device with read-only filesystem. */ if (prepare_and_mount_ro_fs(NULL, tst_test->mntpoint, "tmpfs")) { tst_res(TINFO, "Can't mount tmpfs read-only, " "falling back to block device..."); tst_test->needs_device = 1; tst_test->format_device = 1; } } static int prepare_and_mount_ro_fs(const char *dev, const char *mntpoint, const char *fs_type) { char buf[PATH_MAX]; if (mount(dev, mntpoint, fs_type, 0, NULL)) { tst_res(TINFO | TERRNO, "Can't mount %s at %s (%s)", dev, mntpoint, fs_type); return 1; } mntpoint_mounted = 1; snprintf(buf, sizeof(buf), "%s/dir/", mntpoint); SAFE_MKDIR(buf, 0777); snprintf(buf, sizeof(buf), "%s/file", mntpoint); SAFE_FILE_PRINTF(buf, "file content"); SAFE_CHMOD(buf, 0777); SAFE_MOUNT(dev, mntpoint, fs_type, MS_REMOUNT | MS_RDONLY, NULL); return 0; } > Good point, I completely overlook .needs_rofs. That makes things clearer. > ATM we have 3 other tests in syscalls/unlink. Not sure if all are filesystem > specific (I would say yes, but not sure), but at least unlink05.c (tests > deleting with unlink()) should be tested .all_filesystems. unlink07.c and > unlink08.c test errno. > Kind regards, > Petr
On 05. 06. 24 14:27, Petr Vorel wrote: > Hi Martin, > > Also I wonder if having functionality for .all_filesystems + .needs_rofs > wouldn't be useful. @Cyril @Martin WDYT? > > Also, there is fallback when prepare_and_mount_ro_fs() fails to use block > device. Although, I don't see the read only mount flags added in this fallback, > IMHO MS_RDONLY is only in prepare_and_mount_ro_fs(), therefore the fallback is > read write and we even didn't get TWARN, just plain TINFO (it should be either > TWARN or TINFO with "WARNING:" at least). It would be useful and prepare_device() implements everything that's needed for it but there's a bug in do_setup() which creates a conflict between the two attributes. The .all_filesystems attribute forces .needs_device but a few lines below that is a check that .needs_rofs and .needs_device are not set at the same time. This can be fixed.
> On 05. 06. 24 14:27, Petr Vorel wrote: > > Hi Martin, > > Also I wonder if having functionality for .all_filesystems + .needs_rofs > > wouldn't be useful. @Cyril @Martin WDYT? > > Also, there is fallback when prepare_and_mount_ro_fs() fails to use block > > device. Although, I don't see the read only mount flags added in this fallback, > > IMHO MS_RDONLY is only in prepare_and_mount_ro_fs(), therefore the fallback is > > read write and we even didn't get TWARN, just plain TINFO (it should be either > > TWARN or TINFO with "WARNING:" at least). > It would be useful and prepare_device() implements everything that's needed > for it but there's a bug in do_setup() which creates a conflict between the > two attributes. The .all_filesystems attribute forces .needs_device but a > few lines below that is a check that .needs_rofs and .needs_device are not > set at the same time. This can be fixed. Thanks for info, I'll have look into it. Anyway, you all agreed that splitting the test is needed either way. And because of other thing Martin found (the third unfixed SAFE_OPEN() in verify_unlink()) I'm setting this as changes requested. Kind regards, Petr
Hi! > > It would be useful and prepare_device() implements everything that's needed > > for it but there's a bug in do_setup() which creates a conflict between the > > two attributes. The .all_filesystems attribute forces .needs_device but a > > few lines below that is a check that .needs_rofs and .needs_device are not > > set at the same time. This can be fixed. > > Thanks for info, I'll have look into it. It may not be needed. The counter argument is that if you mix a read only filesystem tests and all_filesystems in one test you are combining two unrelated things that are probably better to be done in a separate tests.
On 05. 06. 24 15:44, Cyril Hrubis wrote: > It may not be needed. The counter argument is that if you mix a read > only filesystem tests and all_filesystems in one test you are combining > two unrelated things that are probably better to be done in a separate > tests. I don't follow that argument. The expected result is that each available filesystem will be mounted read-only. How is that unrelated? The default .needs_rofs setup which mounts tmpfs as the single filesystem should be skipped in this case.
> Hi! > > > It would be useful and prepare_device() implements everything that's needed > > > for it but there's a bug in do_setup() which creates a conflict between the > > > two attributes. The .all_filesystems attribute forces .needs_device but a > > > few lines below that is a check that .needs_rofs and .needs_device are not > > > set at the same time. This can be fixed. > > Thanks for info, I'll have look into it. > It may not be needed. The counter argument is that if you mix a read > only filesystem tests and all_filesystems in one test you are combining > two unrelated things that are probably better to be done in a separate > tests. I thought that we have use case for testing all (or most) filesystems in read only mode, instead testing just whatever is in the default /tmp. But sure, I'll will not do that unless others think it's an useful feature. So does it mean that all read only filesystems behave the same? So far we have 14 tests with .needs_rofs testcases/kernel/syscalls/access/access04.c testcases/kernel/syscalls/acct/acct01.c testcases/kernel/syscalls/chmod/chmod06.c testcases/kernel/syscalls/chown/chown04.c testcases/kernel/syscalls/creat/creat06.c testcases/kernel/syscalls/fchmod/fchmod06.c testcases/kernel/syscalls/fchown/fchown04.c testcases/kernel/syscalls/link/link08.c testcases/kernel/syscalls/mkdir/mkdir03.c testcases/kernel/syscalls/mkdirat/mkdirat02.c testcases/kernel/syscalls/rmdir/rmdir02.c testcases/kernel/syscalls/unlink/unlink09.c testcases/kernel/syscalls/utime/utime06.c testcases/kernel/syscalls/utimes/utimes01.c From these at least mkdir, rmdir, link, unlink have member in struct inode_operations [1] (thus custom operation per filesystem, right?) I suppose update_time might be related to utime* tests. There are other members, but for these we IMHO don't test wit rofs. Then, there is struct file_operations with open, read, write, {compat,unlocked}_ioctl, llseek, ... operations which (at least open) are used in various tests. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/fs.h#n2062 Kind regards, Petr
> On 05. 06. 24 15:44, Cyril Hrubis wrote: > > It may not be needed. The counter argument is that if you mix a read > > only filesystem tests and all_filesystems in one test you are combining > > two unrelated things that are probably better to be done in a separate > > tests. > I don't follow that argument. The expected result is that each available > filesystem will be mounted read-only. How is that unrelated? The default > .needs_rofs setup which mounts tmpfs as the single filesystem should be > skipped in this case. +1 Kind regards, Petr
On Wed, Jun 05, 2024 at 08:57:55AM GMT, Petr Vorel wrote: > > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f > I see useful b4 feature :). > > > change-id: 20240604-unlink09-dc4802f872f9 > But I wonder what is this for (what tool use change-id). This allows you to reliably find all revisions of the same series, for example to do range-diffs between them. -K
Hi Konstantin, > On Wed, Jun 05, 2024 at 08:57:55AM GMT, Petr Vorel wrote: > > > base-commit: 66517b89141fc455ed807f3b95e5260dcf9fb90f > > I see useful b4 feature :). > > > change-id: 20240604-unlink09-dc4802f872f9 > > But I wonder what is this for (what tool use change-id). > This allows you to reliably find all revisions of the same series, for example > to do range-diffs between them. Welcome to LTP ML :). Thank you for giving a hint, it forced me to look up your docs ([1] if anybody interested). BTW it'd be great if patchwork supported it (somebody asked for something similar [2]. I noticed other things - having version in message-id and having somehow "encrypted" email part before "at" (@), but keep part after "at" the same looks interesting. I should find time to explore b4 :). Kind regards, Petr [1] https://b4.docs.kernel.org/en/latest/contributor/prep.html#working-with-series-dependencies [2] https://github.com/getpatchwork/patchwork/issues/583 > -K
diff --git a/testcases/kernel/syscalls/unlink/unlink09.c b/testcases/kernel/syscalls/unlink/unlink09.c index cc4b4a07e..ed6f0adc3 100644 --- a/testcases/kernel/syscalls/unlink/unlink09.c +++ b/testcases/kernel/syscalls/unlink/unlink09.c @@ -11,6 +11,8 @@ * * - EPERM when target file is marked as immutable or append-only * - EROFS when target file is on a read-only filesystem. + * + * Test won't be executed if inode attributes are not supported. */ #include <sys/ioctl.h> @@ -22,8 +24,8 @@ #define DIR_EROFS "erofs" #define TEST_EROFS "erofs/test_erofs" -static int fd_immutable; -static int fd_append_only; +static int fd_immutable = -1; +static int fd_append_only = -1; static struct test_case_t { char *filename; @@ -43,12 +45,18 @@ static void setup(void) { int attr; - fd_immutable = SAFE_OPEN(TEST_EPERM_IMMUTABLE, O_CREAT, 0600); - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); + fd_immutable = SAFE_CREAT(TEST_EPERM_IMMUTABLE, 0600); + TEST(ioctl(fd_immutable, FS_IOC_GETFLAGS, &attr)); + + if (TST_RET == -1 && TST_ERR == ENOTTY) { + SAFE_CLOSE(fd_immutable); + tst_brk(TCONF | TTERRNO, "Inode attributes not supported"); + } + attr |= FS_IMMUTABLE_FL; SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); - fd_append_only = SAFE_OPEN(TEST_EPERM_APPEND_ONLY, O_CREAT, 0600); + fd_append_only = SAFE_CREAT(TEST_EPERM_APPEND_ONLY, 0600); SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); attr |= FS_APPEND_FL; SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); @@ -58,15 +66,19 @@ static void cleanup(void) { int attr; - SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); - attr &= ~FS_IMMUTABLE_FL; - SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); - SAFE_CLOSE(fd_immutable); + if (fd_immutable != -1) { + SAFE_IOCTL(fd_immutable, FS_IOC_GETFLAGS, &attr); + attr &= ~FS_IMMUTABLE_FL; + SAFE_IOCTL(fd_immutable, FS_IOC_SETFLAGS, &attr); + SAFE_CLOSE(fd_immutable); + } - SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); - attr &= ~FS_APPEND_FL; - SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); - SAFE_CLOSE(fd_append_only); + if (fd_append_only != -1) { + SAFE_IOCTL(fd_append_only, FS_IOC_GETFLAGS, &attr); + attr &= ~FS_APPEND_FL; + SAFE_IOCTL(fd_append_only, FS_IOC_SETFLAGS, &attr); + SAFE_CLOSE(fd_append_only); + } } static void verify_unlink(unsigned int i)