On Wed, Feb 08, 2023 at 03:52:40PM -0800, Andrii Nakryiko wrote:
SNIP
> > diff --git a/include/linux/buildid.h b/include/linux/buildid.h
> > index 3b7a0ff4642f..7c818085ad2c 100644
> > --- a/include/linux/buildid.h
> > +++ b/include/linux/buildid.h
> > @@ -3,9 +3,15 @@
> > #define _LINUX_BUILDID_H
> >
> > #include <linux/mm_types.h>
> > +#include <linux/slab.h>
> >
> > #define BUILD_ID_SIZE_MAX 20
> >
> > +struct build_id {
> > + u32 sz;
> > + char data[BUILD_ID_SIZE_MAX];
>
> don't know if 21 vs 24 matters for kmem_cache_create(), but we don't
> need 4 bytes to store build_id size, given max size is 20, so maybe
> use u8 for sz?
ok
>
> > +};
> > +
> > int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> > __u32 *size);
> > int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
> > @@ -17,4 +23,15 @@ void init_vmlinux_build_id(void);
> > static inline void init_vmlinux_build_id(void) { }
> > #endif
> >
> > +#ifdef CONFIG_FILE_BUILD_ID
> > +void __init build_id_init(void);
> > +void build_id_free(struct build_id *bid);
> > +int vma_get_build_id(struct vm_area_struct *vma, struct build_id **bidp);
> > +void file_build_id_free(struct file *f);
> > +#else
> > +static inline void __init build_id_init(void) { }
> > +static inline void build_id_free(struct build_id *bid) { }
> > +static inline void file_build_id_free(struct file *f) { }
> > +#endif /* CONFIG_FILE_BUILD_ID */
> > +
> > #endif
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index c1769a2c5d70..9ad5e5fbf680 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -975,6 +975,9 @@ struct file {
> > struct address_space *f_mapping;
> > errseq_t f_wb_err;
> > errseq_t f_sb_err; /* for syncfs */
> > +#ifdef CONFIG_FILE_BUILD_ID
> > + struct build_id *f_bid;
>
> naming nit: anything wrong with f_buildid or f_build_id? all the
> related APIs use fully spelled out "build_id"
ok
SNIP
> > diff --git a/mm/mmap.c b/mm/mmap.c
> > index 425a9349e610..a06f744206e3 100644
> > --- a/mm/mmap.c
> > +++ b/mm/mmap.c
> > @@ -2530,6 +2530,7 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > pgoff_t vm_pgoff;
> > int error;
> > MA_STATE(mas, &mm->mm_mt, addr, end - 1);
> > + struct build_id *bid = NULL;
> >
> > /* Check against address space limit. */
> > if (!may_expand_vm(mm, vm_flags, len >> PAGE_SHIFT)) {
> > @@ -2626,6 +2627,13 @@ unsigned long mmap_region(struct file *file, unsigned long addr,
> > if (error)
> > goto unmap_and_free_vma;
> >
> > +#ifdef CONFIG_FILE_BUILD_ID
> > + if (vma->vm_flags & VM_EXEC && !file->f_bid) {
> > + error = vma_get_build_id(vma, &bid);
> > + if (error)
> > + goto close_and_free_vma;
>
> do we want to fail mmap_region() if we get -ENOMEM from
> vma_get_build_id()? can't we just store ERR_PTR(error) in f_bid field?
> So we'll have f_bid == NULL for non-exec files, ERR_PTR() for when we
> tried and failed to get build ID, and a valid pointer if we succeeded?
I guess we can do that.. might be handy for debugging
also build_id_parse might fail on missing build id, so you're right,
we should not fail mmap_region in here
thanks,
jirka
