| Message ID | 1ebab7e7-f7ee-b910-9cc8-5d826eee8e97@schaufler-ca.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Smack: Restore the smackfsdef mount option | expand |
On 5/20/2019 3:48 PM, Casey Schaufler wrote: > The 5.1 mount system rework changed the smackfsdef mount option > to smackfsdefault. This fixes the regression by making smackfsdef > treated the same way as smackfsdefault. The change was made in > commit c3300aaf95fb4 from Al Viro. > > Reported-by: Jose Bollo <jose.bollo@iot.bzh> > Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> Al, Dave, is this patch in keeping with the intent of the mount rework? Is there a different way I should do it? Do you want to take it as a fix for the mount work, or should I push it? Thank you. > --- > ??security/smack/smack_lsm.c | 2 ++ > ??1 file changed, 2 insertions(+) > > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c > index b9abcdb36a73..915cf598e164 100644 > --- a/security/smack/smack_lsm.c > +++ b/security/smack/smack_lsm.c > @@ -68,6 +68,7 @@ static struct { > ???????? int len; > ???????? int opt; > ??} smk_mount_opts[] = { > +?????? {"smackfsdef", sizeof("smackfsdef") - 1, Opt_fsdefault}, > ???????? A(fsdefault), A(fsfloor), A(fshat), A(fsroot), A(fstransmute) > ??}; > ??#undef A > @@ -682,6 +683,7 @@ static int smack_fs_context_dup(struct fs_context > *fc, > ??} > > ??static const struct fs_parameter_spec smack_param_specs[] = { > +?????? fsparam_string("fsdef",?????????????? Opt_fsdefault), > ???????? fsparam_string("fsdefault",?????? Opt_fsdefault), > ???????? fsparam_string("fsfloor",?????? Opt_fsfloor), > ???????? fsparam_string("fshat",?????????????? Opt_fshat), >
Casey Schaufler <casey@schaufler-ca.com> wrote: > The change was made in commit c3300aaf95fb4 from Al Viro. This should be in a "Fixes:" tag? > + fsparam_string("fsdef", Opt_fsdefault), > fsparam_string("fsdefault", Opt_fsdefault), > fsparam_string("fsfloor", Opt_fsfloor), > fsparam_string("fshat", Opt_fshat), Would it be better to delete the "fsdefault" line? Also, should all of these be prefixed with "smack"? So: fsparam_string("smackfsdef", Opt_fsdefault), fsparam_string("smackfsfloor", Opt_fsfloor), fsparam_string("smackfshat", Opt_fshat), David
On 5/28/2019 5:23 AM, David Howells wrote: > Casey Schaufler <casey@schaufler-ca.com> wrote: > >> The change was made in commit c3300aaf95fb4 from Al Viro. > This should be in a "Fixes:" tag? Thanks. I wasn't sure how to properly apply that. > >> + fsparam_string("fsdef", Opt_fsdefault), >> fsparam_string("fsdefault", Opt_fsdefault), >> fsparam_string("fsfloor", Opt_fsfloor), >> fsparam_string("fshat", Opt_fshat), > Would it be better to delete the "fsdefault" line? If it hadn't slipped into the 5.1 release I would say to remove it, but now it would be a regression. > > Also, should all of these be prefixed with "smack"? So: > > fsparam_string("smackfsdef", Opt_fsdefault), > fsparam_string("smackfsfloor", Opt_fsfloor), > fsparam_string("smackfshat", Opt_fshat), No. smack_fs_parameters takes care of that. > > David
Casey Schaufler <casey@schaufler-ca.com> wrote: > > Also, should all of these be prefixed with "smack"? So: > > > > fsparam_string("smackfsdef", Opt_fsdefault), > > fsparam_string("smackfsfloor", Opt_fsfloor), > > fsparam_string("smackfshat", Opt_fshat), > > No. smack_fs_parameters takes care of that. It does? *Blink*. smack_fs_parameters.name is just for decorating messages, if that's what you're looking at. David
On 5/28/2019 9:22 AM, David Howells wrote: > Casey Schaufler <casey@schaufler-ca.com> wrote: > >>> Also, should all of these be prefixed with "smack"? So: >>> >>> fsparam_string("smackfsdef", Opt_fsdefault), >>> fsparam_string("smackfsfloor", Opt_fsfloor), >>> fsparam_string("smackfshat", Opt_fshat), >> No. smack_fs_parameters takes care of that. > It does? *Blink*. Well, something does. I can't say that I 100% understand all of how the new mount code handles the mount options. Y'all made sweeping changes, and the code works the way it used to except for the awkward change from smackfsdef to smackfsdefault. It took no small amount of head scratching and experimentation to convince myself that the fix I proposed was correct. > > smack_fs_parameters.name is just for decorating messages, if that's what > you're looking at. > > David
Casey Schaufler <casey@schaufler-ca.com> wrote: > > Casey Schaufler <casey@schaufler-ca.com> wrote: > > > >>> Also, should all of these be prefixed with "smack"? So: > >>> > >>> fsparam_string("smackfsdef", Opt_fsdefault), > >>> fsparam_string("smackfsfloor", Opt_fsfloor), > >>> fsparam_string("smackfshat", Opt_fshat), > >> No. smack_fs_parameters takes care of that. > > It does? *Blink*. > > Well, something does. I can't say that I 100% understand all > of how the new mount code handles the mount options. Y'all made > sweeping changes, and the code works the way it used to except > for the awkward change from smackfsdef to smackfsdefault. It > took no small amount of head scratching and experimentation to > convince myself that the fix I proposed was correct. Ah... I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix for an unconverted filesystem, but smack_fs_context_parse_param() doesn't (which it shouldn't). Can you try grabbing my mount-api-viro branch from: https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git and testing setting smack options on a tmpfs filesystem? You might need to try modifying samples/vfs/test-fsmount.c to make it mount a trmpfs filesystem through the new mount UAPI. David
On 5/28/2019 11:54 AM, David Howells wrote: > Casey Schaufler <casey@schaufler-ca.com> wrote: > >>> Casey Schaufler <casey@schaufler-ca.com> wrote: >>> >>>>> Also, should all of these be prefixed with "smack"? So: >>>>> >>>>> fsparam_string("smackfsdef", Opt_fsdefault), >>>>> fsparam_string("smackfsfloor", Opt_fsfloor), >>>>> fsparam_string("smackfshat", Opt_fshat), >>>> No. smack_fs_parameters takes care of that. >>> It does? *Blink*. >> Well, something does. I can't say that I 100% understand all >> of how the new mount code handles the mount options. Y'all made >> sweeping changes, and the code works the way it used to except >> for the awkward change from smackfsdef to smackfsdefault. It >> took no small amount of head scratching and experimentation to >> convince myself that the fix I proposed was correct. > Ah... I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix > for an unconverted filesystem, but smack_fs_context_parse_param() doesn't > (which it shouldn't). > > Can you try grabbing my mount-api-viro branch from: > > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git > > and testing setting smack options on a tmpfs filesystem? My fedora system won't boot because smackfsdef isn't recognized. :( I will put in my fix and retry. > > You might need to try modifying samples/vfs/test-fsmount.c to make it mount a > trmpfs filesystem through the new mount UAPI. > > David
On 5/28/2019 12:57 PM, Casey Schaufler wrote: > On 5/28/2019 11:54 AM, David Howells wrote: >> Casey Schaufler <casey@schaufler-ca.com> wrote: >> >>>> Casey Schaufler <casey@schaufler-ca.com> wrote: >>>> >>>>>> Also, should all of these be prefixed with "smack"? So: >>>>>> >>>>>> fsparam_string("smackfsdef", Opt_fsdefault), >>>>>> fsparam_string("smackfsfloor", Opt_fsfloor), >>>>>> fsparam_string("smackfshat", Opt_fshat), >>>>> No. smack_fs_parameters takes care of that. >>>> It does? *Blink*. >>> Well, something does. I can't say that I 100% understand all >>> of how the new mount code handles the mount options. Y'all made >>> sweeping changes, and the code works the way it used to except >>> for the awkward change from smackfsdef to smackfsdefault. It >>> took no small amount of head scratching and experimentation to >>> convince myself that the fix I proposed was correct. >> Ah... I suspect the issue is that smack_sb_eat_lsm_opts() strips the prefix >> for an unconverted filesystem, but smack_fs_context_parse_param() doesn't >> (which it shouldn't). >> >> Can you try grabbing my mount-api-viro branch from: >> >> https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git >> >> and testing setting smack options on a tmpfs filesystem? > My fedora system won't boot because smackfsdef isn't recognized. :( > I will put in my fix and retry. No joy there, either. Now it accepts smackfsdef, but doesn't recognize smackfsroot. I don't have this problem with vanilla 5.1. > >> You might need to try modifying samples/vfs/test-fsmount.c to make it mount a >> trmpfs filesystem through the new mount UAPI. >> >> David
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index b9abcdb36a73..915cf598e164 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -68,6 +68,7 @@ static struct { int len; int opt; } smk_mount_opts[] = { + {"smackfsdef", sizeof("smackfsdef") - 1, Opt_fsdefault}, A(fsdefault), A(fsfloor), A(fshat), A(fsroot), A(fstransmute) }; #undef A @@ -682,6 +683,7 @@ static int smack_fs_context_dup(struct fs_context *fc, } static const struct fs_parameter_spec smack_param_specs[] = { + fsparam_string("fsdef", Opt_fsdefault), fsparam_string("fsdefault", Opt_fsdefault), fsparam_string("fsfloor", Opt_fsfloor), fsparam_string("fshat", Opt_fshat),
The 5.1 mount system rework changed the smackfsdef mount option to smackfsdefault. This fixes the regression by making smackfsdef treated the same way as smackfsdefault. The change was made in commit c3300aaf95fb4 from Al Viro. Reported-by: Jose Bollo <jose.bollo@iot.bzh> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com> --- security/smack/smack_lsm.c | 2 ++ 1 file changed, 2 insertions(+)