On 02/07/2018 03:17 PM, Paul Moore wrote:
Hi Tom,
Thanks for the patches, I obviously haven't had a chance to look at
them in detail, but I wanted to provide some quick
feedback/confirmation as well as ask a few questions; in no particular
order:
No worries. Thanks for the quick turnaround!
* You are correct, we need kernel support before we can consider
merging the libseccomp code into the master branch. However, if you
feel it would be helpful to have libseccomp support in order to
test/verify the kernel support, I would consider merging the code into
a development branch.
Thanks for being flexible. I don't think we will need to
commit the libseccomp changes prior to the kernel changes.
But I felt it was important to make the libseccomp changes
(or at least the general idea of the changes) available
prior to sending out some possibly cryptic kernel changes.
Hopefully this will make it easier for the kernel side to
understand the desired goal.
* My apologies on the collision with the "massive src/db.c rework".
That was a bit of a special case patch and I only did it that way
because I didn't believe anyone else had any pending work that
involved that code. If there is anything I can do to help here, let
me know.
It's 100% my fault. The seccomp code base has been so stable,
that I grew lax and didn't check for updates like I should
have. It shouldn't be too hard to straighten, but I'll ping
you if I have any questions. Thanks.
* Testing is a pretty hard requirement for libseccomp, especially
these patches. I'm going to ask for +90% test coverage of this new
code (we're currently at ~92%); I know that's a lot, but this is
critical code.
I 100% agree. I hesitated to send the RFC out without tests,
but I didn't want to write tests for code that may change
significantly before going upstream.
* Improved performance is always good, but functional equivalency and
compatibility with the existing API/callers is another hard
requirement. I mention this because of your comments regarding
argument filtering and hybrid BPF filters, can you elaborate a bit
more on the possible solutions to ensure this works under EBPF
(including hybrid options)?
Agreed. We cannot afford to break existing seccomp users.
So this is purely theoretical, but I think it should work. We could
utilize seccomp's ability in the kernel to run multiple seccomp filters.
The first filter to run would be the EBPF filter. If it returns an
action to run, we could bail out of the for() loop in seccomp_run_filters()
early. If not, continue running the cBPF filter(s) as usual. Existing
seccomp users wouldn't even load an EBPF filter so their behavior would
remain unchanged.
On the libseccomp side, I would expect something like this:
int sys_filter_load(struct db_filter_col *col)
{
int rc;
/* load the classic BPF filter first */
rc = sys_bpf_filter_load(col);
if (rc < 0)
return rc;
/* process the EBPF filter. note - it will only load a
* filter if it has been instructed to do so by the user.
* for classic seccomp filters this is effectively a nop
*/
rc = sys_ebpf_filter_load(col);
return rc;
}
And on the kernel side, I would propose a change like this:
static u32 seccomp_run_filters(const struct seccomp_data *sd,
struct seccomp_filter **match)
{
...
for (; f; f = f->prev)
{
u32 cur_ret = BPF_PROG_RUN(f->prog, sd);
+ if (DATA_ONLY(cur_ret) == SECCOMP_RET_DATA_BREAK)
+ {
+ ret = cur_ret & ~SECCOMP_RET_DATA_BREAK;
+ *match = f;
+ break;
+ }
if (ACTION_ONLY(cur_ret) < ACTION_ONLY(ret))
{
ret = cur_ret;
*match = f; }
}
...
The EBPF code I added to src/gen_ebpf.c could set the
SECCOMP_RET_DATA_BREAK bit in the data section of the action
to force an early exit from the above for() loop.
the data porti
--
You received this message because you are subscribed to the Google Groups
"libseccomp" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To post to this group, send email to [email protected].
For more options, visit https://groups.google.com/d/optout.
