mbox series

[RFC,0/5] Improve static call NULL handling

Message ID cover.1678474914.git.jpoimboe@kernel.org
Headers
Series Improve static call NULL handling |

Message

Josh Poimboeuf March 10, 2023, 8:31 p.m. UTC 
  Static calling a NULL pointer is a NOP, unless you're one of those poor
souls running on an arch (or backported x86 monstrosity) with
CONFIG_HAVE_STATIC_CALL=n, then it's a panic.

The "fix" for this undefined behavior is to tell the user to just use
static_call_cond() instead, if they want consistent NOP behavior.  But
forgetting to do that is likely to cause subtle bugs.  It actually
already did (during RHEL development).

There are two ways to make it consistent:

  a) Make static_call(NULL) a NOP for all configs; or

  b) Make static_call(NULL) a panic for all configs.

Do (a) because it's consistent with the existing HAVE_STATIC_CALL
behavior.  Also it seems simpler to implement and use, and based on
looking at the existing use cases, it's common to want the "do nothing
and return 0" behavior by default.

Then take it a step further and get rid of the distinction between
STATIC_CALL_NULL and STATIC_CALL_RET0.

The end result is less confusing semantics and simpler code all around.


EPILOGUE
--------

If any users wanted panic-on-NULL by default instead of NOP-on-NULL,
that could be added on top of this.  They could just initialize the
static call with a __static_call_bug() helper.

  void __static_call_bug(void)
  {
  	BUG();
  }
  ..
  DEFINE_STATIC_CALL(foo, (func_type)__static_call_bug);

We could take that even further:

  DEFINE_STATIC_CALL_NOP(foo, func_type);
  DEFINE_STATIC_CALL_BUG(bar, func_type);
  ...
  #define STATIC_CALL_NOP (func_type)__static_call_nop
  #define STATIC_CALL_BUG (func_type)__static_call_bug
  ...
  static_call_update(foo, STATIC_CALL_NOP); // do nothing and return 0
  static_call_update(foo, STATIC_CALL_BUG); // panic
  static_call_update(foo, NULL);	    // ???

The default behavior for NULL could be a key-specific policy, stored as
a flag in the static_call_key struct.

The key-specific policy would be easier to deal with than the
call-site-specific policy we have today with static_call_cond().



Josh Poimboeuf (5):
  static_call: Make NULL static calls consistent
  static_call: Make NULL static calls return 0
  static_call: Remove static_call_cond() and its usages
  static_call: Remove DEFINE_STATIC_CALL_RET0() and its uses
  x86/kvm: Simplify static call handling

 arch/powerpc/include/asm/static_call.h    |   1 -
 arch/powerpc/kernel/irq.c                 |   2 +-
 arch/x86/events/amd/core.c                |   2 +-
 arch/x86/events/core.c                    |  26 ++---
 arch/x86/include/asm/kvm-x86-ops.h        |  86 +++++++-------
 arch/x86/include/asm/kvm-x86-pmu-ops.h    |  17 +--
 arch/x86/include/asm/kvm_host.h           |   6 +-
 arch/x86/include/asm/static_call.h        |   8 --
 arch/x86/kvm/irq.c                        |   2 +-
 arch/x86/kvm/lapic.c                      |  22 ++--
 arch/x86/kvm/pmu.c                        |  11 +-
 arch/x86/kvm/x86.c                        |  36 +++---
 include/linux/static_call.h               | 131 +++++-----------------
 kernel/events/core.c                      |   8 +-
 kernel/sched/core.c                       |  10 +-
 security/keys/trusted-keys/trusted_core.c |   2 +-
 16 files changed, 126 insertions(+), 244 deletions(-)

Comments

Steven Rostedt March 10, 2023, 9:09 p.m. UTC | #1 
On Fri, 10 Mar 2023 12:31:12 -0800
Josh Poimboeuf <jpoimboe@kernel.org> wrote:

>   static_call_update(foo, STATIC_CALL_NOP); // do nothing and return 0
>   static_call_update(foo, STATIC_CALL_BUG); // panic
>   static_call_update(foo, NULL);	    // ???
> 
> The default behavior for NULL could be a key-specific policy, stored as
> a flag in the static_call_key struct.

Could we just get rid of the ambiguity and make

 static_call_update(foo, NULL);

trigger a WARN_ON() instead, and always do nop?

The issue I have with allowing NULL, is that it's not easy to know from the
call site what it does.

-- Steve