Messages in this thread | | | Date | Thu, 21 Sep 2017 13:31:50 +0200 | From | Oleg Nesterov <> | Subject | Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() |
| |
On 09/20, Kees Cook wrote: > > I like doing these sanity checks -- this isn't fast-path at all.
Yes, but see another "introduce get_nth_filter()" cleanup I sent, it is similar but more suitable for Tycho's "retrieving seccomp flags" patch.
> > + for (filter = orig; count > 1; filter = filter->prev) ^^^^^^^^^ I just noticed that I forgot to replace this check with "count != 1". Correctness wise this doesn't matter, but looks more clean.
> > count--; > > - } > > - > > - if (WARN_ON(count != 1 || !filter)) { > > - /* The filter tree shouldn't shrink while we're using it. */ > > - ret = -ENOENT; > > - goto out; > > - } > > Similarly, there's no reason to remove this check either.
Well, I disagree, but this is subjective so I won't insist.
Why do we want this WARN_ON() ? The sanity check can only fail if we have a bug in 10 lines above. Lets look at the code after this cleanup,
count = 0; for (filter = orig; filter; filter = filter->prev) count++;
if (filter_off >= count) goto out;
count -= filter_off; for (filter = orig; count != 1; filter = filter->prev) count--;
Do we want to check "count == 1" after the 2nd loop? I don't think so. filter != NULL ? IMO makes no sense. Again, it can only be NULL if the quoted code above is wrong, and in this case the next line
refcount_inc(&filter->usage);
will crash.
Oleg.
| |