Applying rules file fails with `smackload`

[jarkkojs]

Applying these rules fails http://pastebin.com/k9yfBz84. Reading succeeds but applying fails.

[jarkkojs]

$ sudo LD_LIBRARY_PATH=$PWD/libsmack/.libs gdb utils/.libs/smackload
GNU gdb (Ubuntu/Linaro 7.4-2012.04-0ubuntu2.1) 7.4-2012.04
Copyright (C) 2012 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.  Type "show copying"
and "show warranty" for details.
This GDB was configured as "x86_64-linux-gnu".
For bug reporting instructions, please see:
<http://bugs.launchpad.net/gdb-linaro/>...
Reading symbols from /home/jsakkine/devel/smack-jarkkos/utils/.libs/smackload...done.
(gdb) b libsmack.c:679
No source file named libsmack.c.
Make breakpoint pending on future shared library load? (y or [n]) y
Breakpoint 1 (libsmack.c:679) pending.
(gdb) run ../accesses.d/com.samsung.swiftkey-lpm.rule
Starting program: /home/jsakkine/devel/smack-jarkkos/utils/.libs/smackload ../accesses.d/com.samsung.swiftkey-lpm.rule
warning: no loadable sections found in added symbol-file system-supplied DSO at 0x7ffff7ffa000

Breakpoint 1, accesses_apply (clear=0, handle=<optimized out>) at libsmack.c:679
679             ret = -1;
(gdb) p rule->subject
$1 = "com.samsung.swiftkey-lpm", '\000' <repeats 231 times>
(gdb) p rule->object
$2 = "system::vconf", '\000' <repeats 242 times>
(gdb) p rule->access_type
$3 = "\000\000\000\000\000"
(gdb) p rule->allow_access_type 
$2 = "rwxat"
(gdb) p rule->deny_access_type 
$3 = "-----"

[jarkkojs]

@rafal-krypa: do you have any clue what could cause this? All the data looks right. I just thought you might have some idea because you implemented change-rule, right?

[jarkkojs]

Hey, interesting news. Problem does not seem to prevail in 3.12 kernel. I was running 3.8 kernel before in my Ubuntu machine. Do you have some idea could have fixed that between 3.8 and 3.12? Anyway closing this bug.

[rafal-krypa]

Some more interesting news. The change-rule interface didn't appear in Linux kernel before 3.10. Maybe the error was related to bad handling of missing /smack/change-rule?

[rafal-krypa]

Or it was actually a feature. Function accesses_apply() contains path that returns with -1 if opening change-rule failed and a "modify rule" is on the list. It should probably produce some warning, but failing eventually is the only option there. Does this fit your test results?

[jarkkojs]

I think it does actually. accesses_apply() should fail when it encounters a modify rule when support is not available. it should not produce warning of any kind. Processing should stop. It's an error condition.

[jarkkojs]

I will reopen this bug for fixing this issue. I'm strong believer of failing early. We provide backwards compatibility in a sense that we support kernels that have long labels and no change-rule support but we fail if support does not scale to the given input. That's the only sane compromise here and tradeoff that make sense. You could provide me the fix and I can test your fix with 3.8 kernel.

I underline that this is a generic guideline that applies for all backwards compatibility cases.

[jarkkojs]

Updated https://github.com/smack-team/smack/wiki. I'm slowly forming sane contracts and guidelines for this project.

[rafal-krypa]

I'm not sure why did you reopen this bug. From what I understand, the expected behavior is already there. You ran smackctl apply with modify rules in policy, on kernel with no change-rule interface. Function accesses_apply() failed without producing any warning. If that's what you'd like to have, what should be fixed? The "failing early" scenario could mean failing in smack_accesses_add_modify(), but I'd like to address that under #40.

[jarkkojs]

It should stop processing rules and return with -1 when it encounters a rule that it cannot apply. #40 is not something that should be done as a single issue or pull request. Even such pull request was made, I wouldn't probably take it. This is something that need to be converged over period of time in order to avoid unnecessary regressions.

If a workload is encountered that causes issues, it's a good place to iterate further in that goal. I'm encountering now a workload that needs to fixed. Fixing this in smack_accesses_add_modify() works for me. It's good idea to fail already there. I think that pull request would fit well into this issue.

I think it is obvious why this was reopened and I don't think it is a justification not to fix this separately that there's a bigger plan to fix backwards compatibility. This should fixed now. I wasted couple of days because of this.

[rafal-krypa]

Let's check it with the source code of accesses_apply().

First it tries to open change-rule interface file. Failure doesn't stop the function, because there might be no modify rules on the list. It will proceed in such case with change_fd = -1: libsmack/libsmack.c:642-648.

Then rules are processed one by one. If it finds a modify rule, it will format it with snprintf() and set fd = change_fd: libsmack/libsmack.c:651-656.

Just before sending formatted rule to kernel, accesses_apply() checks if fd < 0: libsmack/libsmack.c:669-672. And this is where it fails in the discussed scenario.

So what's wrong in the current code? Would you like the function to fail few instructions earlier and save one unneeded snprintf()? Or am I missing something?

If on the other hand you'd like it to fail much earlier, in smack_accesses_add_modify(), then I'll need some new mechanism for detecting kernel Smack features. Current code doesn't even try to open change-rule or load2 before API function smack_accesses_apply() is called. There is no information for smack_accesses_add_modify() to make a decision about failing. You wrote under #40, that you prefer run-time detection over compile-time checks, so a new code for such detection must be made. And this is something that IMHO should be done separately and is in fact loosely related to this issue.

[jarkkojs]

Yes, so it seems. Hmm.. what happened to me with 3.8 kernel was that it smackload "succeeded" but no single rule was uploaded. I'll do a retest and report the results. Do you have a chance to try this with 3.8 kernel?

Maybe there's something else wrong.

[jarkkojs]

OK, now I think I see what's wrong. I think to close this urgent issue we should fix in accesses_apply(). The problem is that it should check change_fd also in the loop and fail if it isn't available. OK, better would do the check in add_modify_rule but as you said at the moment there's not infra to do that.

[jarkkojs]

Right. It fails early in 712 :) OK, last time I tried it on 3.8 was succeeding silently with the same rule file. Only explanation is that I had some unfinished change on top of my tree or something like that. This works for me but the control logic is really messy I think I'll see if I could clean it up just a bit without breaking anything. That's the proper fix for this..

[jarkkojs]

I'm preparing a patch where failing is done as first thing when modify rule is encountered and change-rule does not exist. The current loop works but it's hard to debug and is lazy on failing.

[jarkkojs]

I think these changes would improve a little bit of maintainability and readability and would not be too intrusive for the release. What do you think? Nothing major changed..

[jarkkojs]

Thanks. Right. I'll revise path according to your comments. Thanks!

[jarkkojs]

There wasn't previously check for that unlikely case or am I missing something? If data fits into buffer, then snprintf should return equivalent to strlen. I'll add sanity check for LOAD_LEN + 1

[jarkkojs]

I divided patch to patch set that contains logical changes.

[jarkkojs]

There's no unlikely case that rule wouldn't fit into the buffer. There's however case where only 'load' is available. Then label length must be verified.