On Tue, Oct 25, 2022 at 09:10:17AM -0700, Dave Marchevsky wrote:
> This is a followup to a previous commit of mine [0], which added the
> allow_sys_admin_access && capable(CAP_SYS_ADMIN) check. This patch
> rearranges the order of checks in fuse_allow_current_process without
> changing functionality.
>
> [0] added allow_sys_admin_access && capable(CAP_SYS_ADMIN) check to the
> beginning of the function, with the reasoning that
> allow_sys_admin_access should be an 'escape hatch' for users with
> CAP_SYS_ADMIN, allowing them to skip any subsequent checks.
>
> However, placing this new check first results in many capable() calls when
> allow_sys_admin_access is set, where another check would've also
> returned 1. This can be problematic when a BPF program is tracing
> capable() calls.
>
> At Meta we ran into such a scenario recently. On a host where
> allow_sys_admin_access is set but most of the FUSE access is from
> processes which would pass other checks - i.e. they don't need
> CAP_SYS_ADMIN 'escape hatch' - this results in an unnecessary capable()
> call for each fs op. We also have a daemon tracing capable() with BPF and
> doing some data collection, so tracing these extraneous capable() calls
> has the potential to regress performance for an application doing many
> FUSE ops.
>
> So rearrange the order of these checks such that CAP_SYS_ADMIN 'escape
> hatch' is checked last. Add a small helper, fuse_permissible_uidgid, to
> make the logic easier to understand. Previously, if allow_other is set
> on the fuse_conn, uid/git checking doesn't happen as current_in_userns
> result is returned. These semantics are maintained here:
> fuse_permissible_uidgid check only happens if allow_other is not set.
>
> [0]: commit 9ccf47b26b73 ("fuse: Add module param for CAP_SYS_ADMIN access bypassing allow_other")
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx>
> Suggested-by: Andrii Nakryiko <andrii@xxxxxxxxxx>
> Cc: Christian Brauner <brauner@xxxxxxxxxx>
> ---
> v2 -> v3: lore.kernel.org/linux-fsdevel/20221020201409.1815316-1-davemarchevsky@xxxxxx
>
> * Add fuse_permissible_uidgid, rearrange fuse_allow_current_process to
> not use 'goto' (Christian)
>
> v1 -> v2: lore.kernel.org/linux-fsdevel/20221020183830.1077143-1-davemarchevsky@xxxxxx
>
> * Add missing brackets to allow_other if statement (Andrii)
Looks good to me,
Reviewed-by: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
