Re: "warning: context imbalance" in kernel/bpf/hashtab.c

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sat, Nov 7, 2020 at 4:42 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> But isn't a lot of code written to explicitly support this, with an
> argument or some other condition selecting between the locked mode
> or the unlocked mode?

A lot? No. I've actively discouraged it.

It does exist - don't get me wrong - but it shouldn't be the normal pattern.

> This one is fine for Sparse
>         static inline int cond_lock1(void)
>         {
>                 if (complex_cond()) {
>                         take_lock();
>                         minor();
>                         return 1;
>                 }
>                 return 0;
>         }
>         static void ok1(void)
>         {
>                 int cond = cond_lock1();
>                 if (cond) {
>                         major();
>                         drop_lock();
>                 }
>         }

That _is_ a somewhat common case, and it's basically the "first helper
tells the caller that it took the lock".

The _really_ basic version of that is the various "trylock()" things,
which has that "__cond_lock()" macro in the kernel for it, so that
sparse can catch that very special case.

That said, it's still not the _common_ case, which is just that the
code is straightforward

    take_lock();
    do something under the lock
    drop_lock();

> The next one corresponds to the situation Song Liu reported
>         static inline int cond_lock2(void)
>         {
>                 if (!complex_cond())
>                         return -1;
>
>                 take_lock();
>                 minor();
>                 return 0;
>         }
>         static int okish(void)
>         {
>                 int cond = cond_lock2();
>                 if (cond)
>                         return 0;
>                 major();
>                 drop_lock();
>                 return 1;
>         }

Yeah, this is a more complex version of the same.

Clearly sparse doesn't grok it today, but the fact that your patches
make sparse able to track it is a good improvement.

> The one that really annoys me is this one. It's very simple and, as
> far as I know, quite common in the kernel but kinda hopeless.
>         static void ko(int cond)
>         {
>                 if (cond)
>                         take_lock();
>                 major();
>                 if (cond)
>                         drop_lock();
>         }

This really is invalid code. "major()" is done in two different lock contexts.

Sparse _should_ complain.

             Linus