[LTP] [PATCH] save_restore: Check whether path is writable

[Jan Stancek]

On Tue, Oct 25, 2022 at 6:13 PM Martin Doucha <mdoucha@suse.cz> wrote:
>
> On 24. 10. 22 9:16, Jan Stancek wrote:
> > On Fri, Oct 21, 2022 at 10:35 PM Petr Vorel <pvorel@suse.cz> wrote:
> > Problem description makes it sound like this issue affects all 3 types
> > of config options. Isn't the problem affecting only optional config paths?
> >
> > Having entry with "(no prefix)" or "!" in save_restore implies that
> > test wants to write to that path - if we TCONF on root privileges or
> > read/write access probably doesn't make much difference - we can't
> > continue.
> >
> > For "?" prefix, I agree that since its optional, test should be able
> > to run cleanly without root privileges.
>
> It does affect all 3 but slightly differently, depending on the "val"
> field of the respective .save_restore item. The current implementation
> behaves like this without root privileges:
> - (no prefix): If val is NULL, the test will save the data, run the test
> and trigger TWARN at the end.

Is this real scenario? Why is test saving sysfs value, which is then
never changed?
I would expect that in this case, you could drop save_restore entirely.

> If val is not NULL, the test will fail
> immediately after saving sysfile data because it'll try to write into a
> read-only file.

> We'd want TCONF instead in the latter case.
> - '!': Same behavior as with no prefix but we want to keep it.
> - '?': Same behavior as with no prefix. We want either TCONF or to
> ignore the sysfile entirely with a TINFO message.
>
> > For optional path, if test can't read/write it (b/o of no root privileges),
> > I think library shouldn't try to save it - then that would also skip
> > attempt to restore it.
>
> There are be two different kinds of optional paths, though:
> 1) paths that sometimes don't exist but must be written to if they do
> 2) paths that may be left alone if they exist and already contain the
> right value (otherwise TCONF)
>
> So the question is whether I should steal the '?' prefix for type #2 and
> we'll introduce a new prefix later if needed, or whether we'll reserve
> the '?' prefix for type #1 according to current behavior and introduce
> the new prefix now.

The case 2) looks like it could apply to non-optional paths too. So maybe
best option would be to drop "!" and "?" prefixes and turn them into flags/enums
which can be then combined together.

"/proc/sys/kernel/pid_max", 0, val // TCONF if path doesn't exist
"/proc/sys/kernel/pid_max", SR_MUST_EXIST, val // TBROK if path doesn't exist
"/proc/sys/kernel/pid_max", SR_MAY_EXIST, val // if exists, save it
"/proc/sys/kernel/pid_max", SR_CONST_VAL, val // if already has val,
skip saving it
"/proc/sys/kernel/pid_max", SR_MAY_EXIST | SR_CONST_VAL, val // if
exists check it already has val, otherwise save it

What do you think? Would that make it easier to represent/implement all cases?

>
> On 21. 10. 22 22:34, Petr Vorel wrote:
> > Looking at files which use '?', some of them (mostly network related, I guess
> > written/rewritten by Martin) use SAFE_TRY_FILE_PRINTF() on
> > /proc/sys/user/max_user_namespaces. It looks to me these need to to skip
> > read-only files, i.e. define new flag with this behavior.
>
> All those SAFE_TRY_FILE_PRINTF() calls are writing a constant so they
> can be eliminated by filling the second field of the .save_restore
> struct. I'll do that in the follow-up patchset when we agree how to
> implement this part.
>
> --
> Martin Doucha   mdoucha@suse.cz
> QA Engineer for Software Maintenance
> SUSE LINUX, s.r.o.
> CORSO IIa
> Krizikova 148/34
> 186 00 Prague 8
> Czech Republic
>