Re: [PATCH v2 bpf-next 4/9] bpf: add new acquire/release based BPF kfuncs for exe_file

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Mar 06, 2024 at 07:40:00AM +0000, Matt Bobrowski wrote:
> It is rather common for BPF LSM program types to perform the struct
> walk current->mm->exe_file and subsequently operate on fields of the
> backing file. At times, some of these operations involve passing a
> exe_file's field on to BPF helpers and such
> i.e. bpf_d_path(&current->mm->exe_file->f_path). However, doing so
> isn't necessarily always reliable as the backing file that exe_file is
> pointing to may be in the midst of being torn down and handing
> anything contained within this file to BPF helpers and such can lead
> to memory corruption issues [0].
> 
> To alleviate possibly operating on semi-torn down instances of
> current->mm->exe_file we introduce a set of BPF kfuncs that posses
> KF_ACQUIRE/KF_RELEASE based semantics. Such BPF kfuncs will allow BPF
> LSM program types to reliably get/put a reference on a
> current->mm->exe_file.
> 
> The following new BPF kfuncs have been added:
> 
> struct file *bpf_get_task_exe_file(struct task_struct *task);
> struct file *bpf_get_mm_exe_file(struct mm_struct *mm);
> void bpf_put_file(struct file *f);
> 
> Internally, these new BPF kfuncs simply call the preexisting in-kernel
> functions get_task_exe_file(), get_mm_exe_file(), and fput()
> accordingly. From a technical standpoint, there's absolutely no need
> to re-implement such helpers just for BPF as they're currently scoped
> to BPF LSM program types.
> 
> Note that we explicitly do not explicitly rely on the use of very low
> level in-kernel functions like get_file_rcu() and get_file_active() to
> acquire a reference on current->mm->exe_file and such. This is super
> subtle code and we probably want to avoid exposing any such subtleties
> to BPF in the form of BPF kfuncs. Additionally, the usage of a double
> pointer i.e. struct file **, isn't something that the BPF verifier
> currently recognizes nor has any intention to recognize for the
> foreseeable future.
> 
> [0] https://lore.kernel.org/bpf/CAG48ez0ppjcT=QxU-jtCUfb5xQb3mLr=5FcwddF_VKfEBPs_Dg@xxxxxxxxxxxxxx/
> 
> Signed-off-by: Matt Bobrowski <mattbobrowski@xxxxxxxxxx>
> ---

get_mm_exe_file() and get_task_exe_file() aren't even exported to
modules which makes me a lot more hesitant.

>  kernel/trace/bpf_trace.c | 56 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> index 801808b6efb0..539c58db74d7 100644
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -1518,12 +1518,68 @@ __bpf_kfunc void bpf_mm_drop(struct mm_struct *mm)
>  	mmdrop(mm);
>  }
>  
> +/**
> + * bpf_get_task_exe_file - get a reference on the exe_file associated with the
> + * 	       		   mm_struct that is nested within the supplied
> + * 	       		   task_struct
> + * @task: task_struct of which the nested mm_struct's exe_file is to be
> + * referenced
> + *
> + * Get a reference on the exe_file that is associated with the mm_struct nested
> + * within the supplied *task*. A reference on a file pointer acquired by this
> + * kfunc must be released using bpf_put_file(). Internally, this kfunc leans on
> + * get_task_exe_file(), such that calling bpf_get_task_exe_file() would be
> + * analogous to calling get_task_exe_file() outside of BPF program context.
> + *
> + * Return: A referenced pointer to the exe_file associated with the mm_struct
> + * nested in the supplied *task*, or NULL.
> + */
> +__bpf_kfunc struct file *bpf_get_task_exe_file(struct task_struct *task)
> +{
> +	return get_task_exe_file(task);
> +}
> +
> +/**
> + * bpf_get_mm_exe_file - get a reference on the exe_file for the supplied
> + * 			 mm_struct.
> + * @mm: mm_struct of which the exe_file to get a reference on
> + *
> + * Get a reference on the exe_file associated with the supplied *mm*. A
> + * reference on a file pointer acquired by this kfunc must be released using
> + * bpf_put_file(). Internally, this kfunc leans on get_mm_exe_file(), such that
> + * calling bpf_get_mm_exe_file() would be analogous to calling get_mm_exe_file()
> + * outside of BPF program context.
> + *
> + * Return: A referenced file pointer to the exe_file for the supplied *mm*, or
> + * NULL.
> + */
> +__bpf_kfunc struct file *bpf_get_mm_exe_file(struct mm_struct *mm)
> +{
> +	return get_mm_exe_file(mm);
> +}
> +
> +/**
> + * bpf_put_file - put a reference on the supplied file
> + * @f: file of which to put a reference on
> + *
> + * Put a reference on the supplied *f*.
> + */
> +__bpf_kfunc void bpf_put_file(struct file *f)
> +{
> +	fput(f);
> +}

That's probably reasonable because this is already exported to modules.
But that's not a generic argument. So there'll never be kfuncs for
__fput_sync(), flushed_delayed_fput() and other special purpose file put
or get helpers.

Conditio sine qua non is that all such kfunc definitions must live in
the respective file in fs/. They won't be hidden somewhere in
kernel/bpf/.




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux