| Message ID | YTDyE9wVQQBxS77r@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Relax restrictions on user.* xattr | expand |
On Thu, Sep 02, 2021 at 11:47:31AM -0400, Vivek Goyal wrote: > > xfstests: generic/062: Do not run on newer kernels > > This test has been written with assumption that setting user.* xattrs will > fail on symlink and special files. When newer kernels support setting > user.* xattrs on symlink and special files, this test starts failing. > > Found it hard to change test in such a way that it works on both type of > kernels. Primary problem is 062.out file which hardcodes the output and > output will be different on old and new kernels. > > So instead, do not run this test if kernel is new and is expected to > exhibit new behavior. Next patch will create a new test and run that > test on new kernel. > > IOW, on old kernels run 062 and on new kernels run new test. > > This is a proposed patch. Will need to be fixed if corresponding > kernel changes are merged upstream. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > tests/generic/062 | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > Index: xfstests-dev/tests/generic/062 > =================================================================== > --- xfstests-dev.orig/tests/generic/062 2021-08-31 15:51:08.160307982 -0400 > +++ xfstests-dev/tests/generic/062 2021-08-31 16:27:41.678307982 -0400 > @@ -55,6 +55,26 @@ _require_attrs > _require_symlinks > _require_mknod > > +user_xattr_allowed() > +{ > + local kernel_version kernel_patchlevel > + > + kernel_version=`uname -r | awk -F. '{print $1}'` > + kernel_patchlevel=`uname -r | awk -F. '{print $2}'` > + > + # Kernel version 5.14 onwards allow user xattr on symlink/special files. > + [ $kernel_version -lt 5 ] && return 1 > + [ $kernel_patchlevel -lt 14 ] && return 1 > + return 0; > +} We don't do this because code changes get backported to random kernels and so the kernel release is not a reliable indicator of feature support. Probing the functionality is the only way to reliably detect what a kernel supports. That's what we don in all the _requires*() functions, which is what this should all be wrapped in. > +# Kernel version 5.14 onwards allow user xattr on symlink/special files. > +# Do not run this test on newer kernels. Instead run the new test > +# which has been written with the assumption that user.* xattr > +# will succeed on symlink and special files. > +user_xattr_allowed && _notrun "Kernel allows user.* xattrs on symlinks and special files. Skipping this test. Run newer test instead." "run a newer test instead" is not a useful error message. Nor do you need "skipping this test" - that's exactly what "notrun" means. Cheers, Dave.
On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote: > xfstests: generic/062: Do not run on newer kernels > > This test has been written with assumption that setting user.* xattrs will > fail on symlink and special files. When newer kernels support setting > user.* xattrs on symlink and special files, this test starts failing. It's actually a good thing that this test case triggers for the kernel change you're proposing; that change should never be merged. The user.* namespace is meant for data with the same access permissions as the file data, and it has been for many years. We may have applications that assume the existing behavior. In addition, this change would create backwards compatibility problems for things like backups. I'm not convinced that what you're actually proposing (mapping security.selinux to a different attribute name) actually makes sense, but that's a question for the selinux folks to decide. Mapping it to a user.* attribute is definitely wrong though. The modified behavior would affect anybody, not only users of selinux and/or virtiofs. If mapping attribute names is actually the right approach, then you need to look at trusted.* xattrs, which exist specifically for this kind of purpose. You've noted that trusted.* xattrs aren't supported over nfs. That's unfortunate, but not an acceptable excuse for messing up user.* xattrs. Thanks, Andreas
On Thu, Sep 02, 2021 at 11:47:31AM -0400, Vivek Goyal wrote: > > xfstests: generic/062: Do not run on newer kernels > > This test has been written with assumption that setting user.* xattrs will > fail on symlink and special files. When newer kernels support setting > user.* xattrs on symlink and special files, this test starts failing. > > Found it hard to change test in such a way that it works on both type of > kernels. Primary problem is 062.out file which hardcodes the output and > output will be different on old and new kernels. > > So instead, do not run this test if kernel is new and is expected to > exhibit new behavior. Next patch will create a new test and run that > test on new kernel. > > IOW, on old kernels run 062 and on new kernels run new test. > > This is a proposed patch. Will need to be fixed if corresponding > kernel changes are merged upstream. > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com> > --- > tests/generic/062 | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > Index: xfstests-dev/tests/generic/062 > =================================================================== > --- xfstests-dev.orig/tests/generic/062 2021-08-31 15:51:08.160307982 -0400 > +++ xfstests-dev/tests/generic/062 2021-08-31 16:27:41.678307982 -0400 > @@ -55,6 +55,26 @@ _require_attrs > _require_symlinks > _require_mknod > > +user_xattr_allowed() > +{ > + local kernel_version kernel_patchlevel > + > + kernel_version=`uname -r | awk -F. '{print $1}'` > + kernel_patchlevel=`uname -r | awk -F. '{print $2}'` > + > + # Kernel version 5.14 onwards allow user xattr on symlink/special files. > + [ $kernel_version -lt 5 ] && return 1 > + [ $kernel_patchlevel -lt 14 ] && return 1 > + return 0; > +} I don't think this's a good way to judge if run or notrun a test. Many downstream kernels always backport upstream features. I can't say what's the best way to deal with this thing, I only can provide two optional methods: 1) Add new requre_* helpers to check if current kernel support to set xattr on symlink and special files, then let this case only run on support/unsupport condition. 2) Use _link_out_file() to link the .out file to different golden images (refer to generic/050 etc), according to different feature implementation. If anyone has a better method, feel free to talk :) Thanks, Zorro > + > + > +# Kernel version 5.14 onwards allow user xattr on symlink/special files. > +# Do not run this test on newer kernels. Instead run the new test > +# which has been written with the assumption that user.* xattr > +# will succeed on symlink and special files. > +user_xattr_allowed && _notrun "Kernel allows user.* xattrs on symlinks and special files. Skipping this test. Run newer test instead." > + > rm -f $tmp.backup1 $tmp.backup2 $seqres.full > > # real QA test starts here >
On Fri, Sep 3, 2021 at 8:31 AM Andreas Gruenbacher <agruenba@redhat.com> wrote: > On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > xfstests: generic/062: Do not run on newer kernels > > > > This test has been written with assumption that setting user.* xattrs will > > fail on symlink and special files. When newer kernels support setting > > user.* xattrs on symlink and special files, this test starts failing. > > It's actually a good thing that this test case triggers for the kernel > change you're proposing; that change should never be merged. The > user.* namespace is meant for data with the same access permissions as > the file data, and it has been for many years. We may have > applications that assume the existing behavior. In addition, this > change would create backwards compatibility problems for things like > backups. > > I'm not convinced that what you're actually proposing (mapping > security.selinux to a different attribute name) actually makes sense, > but that's a question for the selinux folks to decide. Mapping it to a > user.* attribute is definitely wrong though. The modified behavior > would affect anybody, not only users of selinux and/or virtiofs. If > mapping attribute names is actually the right approach, then you need > to look at trusted.* xattrs, which exist specifically for this kind of > purpose. You've noted that trusted.* xattrs aren't supported over nfs. > That's unfortunate, but not an acceptable excuse for messing up user.* > xattrs. Another possibility would be to make selinux use a different security.* attribute for this nested selinux case. That way, the "host" selinux would retain some control over the labels the "guest" uses. Thanks, Andreas
Well, we could also look at supporting trusted.* xattrs over NFS. I don't know much about them, but it looks like it wouldn't be a lot of work to specify, especially now that we've already got user xattrs? We'd just write a new internet draft that refers to the existing user.* xattr draft for most of the details. --b. On Fri, Sep 3, 2021 at 2:56 AM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > On Fri, Sep 3, 2021 at 8:31 AM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > xfstests: generic/062: Do not run on newer kernels > > > > > > This test has been written with assumption that setting user.* xattrs will > > > fail on symlink and special files. When newer kernels support setting > > > user.* xattrs on symlink and special files, this test starts failing. > > > > It's actually a good thing that this test case triggers for the kernel > > change you're proposing; that change should never be merged. The > > user.* namespace is meant for data with the same access permissions as > > the file data, and it has been for many years. We may have > > applications that assume the existing behavior. In addition, this > > change would create backwards compatibility problems for things like > > backups. > > > > I'm not convinced that what you're actually proposing (mapping > > security.selinux to a different attribute name) actually makes sense, > > but that's a question for the selinux folks to decide. Mapping it to a > > user.* attribute is definitely wrong though. The modified behavior > > would affect anybody, not only users of selinux and/or virtiofs. If > > mapping attribute names is actually the right approach, then you need > > to look at trusted.* xattrs, which exist specifically for this kind of > > purpose. You've noted that trusted.* xattrs aren't supported over nfs. > > That's unfortunate, but not an acceptable excuse for messing up user.* > > xattrs. > > Another possibility would be to make selinux use a different > security.* attribute for this nested selinux case. That way, the > "host" selinux would retain some control over the labels the "guest" > uses. > > Thanks, > Andreas >
On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote: > Well, we could also look at supporting trusted.* xattrs over NFS. I > don't know much about them, but it looks like it wouldn't be a lot of > work to specify, especially now that we've already got user xattrs? > We'd just write a new internet draft that refers to the existing > user.* xattr draft for most of the details. Will be nice if we can support trusted.* xattrs on NFS. Vivek > > --b. > > On Fri, Sep 3, 2021 at 2:56 AM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > > > On Fri, Sep 3, 2021 at 8:31 AM Andreas Gruenbacher <agruenba@redhat.com> wrote: > > > On Thu, Sep 2, 2021 at 5:47 PM Vivek Goyal <vgoyal@redhat.com> wrote: > > > > xfstests: generic/062: Do not run on newer kernels > > > > > > > > This test has been written with assumption that setting user.* xattrs will > > > > fail on symlink and special files. When newer kernels support setting > > > > user.* xattrs on symlink and special files, this test starts failing. > > > > > > It's actually a good thing that this test case triggers for the kernel > > > change you're proposing; that change should never be merged. The > > > user.* namespace is meant for data with the same access permissions as > > > the file data, and it has been for many years. We may have > > > applications that assume the existing behavior. In addition, this > > > change would create backwards compatibility problems for things like > > > backups. > > > > > > I'm not convinced that what you're actually proposing (mapping > > > security.selinux to a different attribute name) actually makes sense, > > > but that's a question for the selinux folks to decide. Mapping it to a > > > user.* attribute is definitely wrong though. The modified behavior > > > would affect anybody, not only users of selinux and/or virtiofs. If > > > mapping attribute names is actually the right approach, then you need > > > to look at trusted.* xattrs, which exist specifically for this kind of > > > purpose. You've noted that trusted.* xattrs aren't supported over nfs. > > > That's unfortunate, but not an acceptable excuse for messing up user.* > > > xattrs. > > > > Another possibility would be to make selinux use a different > > security.* attribute for this nested selinux case. That way, the > > "host" selinux would retain some control over the labels the "guest" > > uses. > > > > Thanks, > > Andreas > > >
On Fri, Sep 3, 2021 at 11:43 AM Vivek Goyal <vgoyal@redhat.com> wrote: > On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote: > > Well, we could also look at supporting trusted.* xattrs over NFS. I > > don't know much about them, but it looks like it wouldn't be a lot of > > work to specify, especially now that we've already got user xattrs? > > We'd just write a new internet draft that refers to the existing > > user.* xattr draft for most of the details. > > Will be nice if we can support trusted.* xattrs on NFS. Maybe I should start a separate thread for that. Who would need to be on it to be sure we get this right? --b.
On 9/3/2021 8:50 AM, Bruce Fields wrote: > On Fri, Sep 3, 2021 at 11:43 AM Vivek Goyal <vgoyal@redhat.com> wrote: >> On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote: >>> Well, we could also look at supporting trusted.* xattrs over NFS. I >>> don't know much about them, but it looks like it wouldn't be a lot of >>> work to specify, especially now that we've already got user xattrs? >>> We'd just write a new internet draft that refers to the existing >>> user.* xattr draft for most of the details. >> Will be nice if we can support trusted.* xattrs on NFS. > Maybe I should start a separate thread for that. Who would need to be > on it to be sure we get this right? I would like to be included. It would probably be a good idea to include the LSM list, linux-security-module@vger.kernel.org. I'll leave the networking and filesystem folks to speak for themselves. > > --b. >
On Fri, Sep 03, 2021 at 11:50:43AM -0400, Bruce Fields wrote: > On Fri, Sep 3, 2021 at 11:43 AM Vivek Goyal <vgoyal@redhat.com> wrote: > > On Fri, Sep 03, 2021 at 10:42:34AM -0400, Bruce Fields wrote: > > > Well, we could also look at supporting trusted.* xattrs over NFS. I > > > don't know much about them, but it looks like it wouldn't be a lot of > > > work to specify, especially now that we've already got user xattrs? > > > We'd just write a new internet draft that refers to the existing > > > user.* xattr draft for most of the details. > > > > Will be nice if we can support trusted.* xattrs on NFS. > > Maybe I should start a separate thread for that. Who would need to be > on it to be sure we get this right? I will like to be on cc list. Vivek
Index: xfstests-dev/tests/generic/062 =================================================================== --- xfstests-dev.orig/tests/generic/062 2021-08-31 15:51:08.160307982 -0400 +++ xfstests-dev/tests/generic/062 2021-08-31 16:27:41.678307982 -0400 @@ -55,6 +55,26 @@ _require_attrs _require_symlinks _require_mknod +user_xattr_allowed() +{ + local kernel_version kernel_patchlevel + + kernel_version=`uname -r | awk -F. '{print $1}'` + kernel_patchlevel=`uname -r | awk -F. '{print $2}'` + + # Kernel version 5.14 onwards allow user xattr on symlink/special files. + [ $kernel_version -lt 5 ] && return 1 + [ $kernel_patchlevel -lt 14 ] && return 1 + return 0; +} + + +# Kernel version 5.14 onwards allow user xattr on symlink/special files. +# Do not run this test on newer kernels. Instead run the new test +# which has been written with the assumption that user.* xattr +# will succeed on symlink and special files. +user_xattr_allowed && _notrun "Kernel allows user.* xattrs on symlinks and special files. Skipping this test. Run newer test instead." + rm -f $tmp.backup1 $tmp.backup2 $seqres.full # real QA test starts here
xfstests: generic/062: Do not run on newer kernels This test has been written with assumption that setting user.* xattrs will fail on symlink and special files. When newer kernels support setting user.* xattrs on symlink and special files, this test starts failing. Found it hard to change test in such a way that it works on both type of kernels. Primary problem is 062.out file which hardcodes the output and output will be different on old and new kernels. So instead, do not run this test if kernel is new and is expected to exhibit new behavior. Next patch will create a new test and run that test on new kernel. IOW, on old kernels run 062 and on new kernels run new test. This is a proposed patch. Will need to be fixed if corresponding kernel changes are merged upstream. Signed-off-by: Vivek Goyal <vgoyal@redhat.com> --- tests/generic/062 | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)