Messages in this thread |  | | From | Pascal Bouchareine <> | Date | Mon, 27 Jul 2020 18:39:38 -0700 | Subject | Re: [PATCH v3] proc,fcntl: introduce F_SET_DESCRIPTION |
| |
Thanks for reviewing, I added some questions inline below
On Mon, Jul 27, 2020 at 7:21 AM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > + d = strndup_user(desc, MAX_FILE_DESC_SIZE); > > This should be kmem accounted because allocation is persistent. > To make things more entertaining, strndup_user() doesn't have gfp_t argument.
I had to look it up so I might be very far from it, but is that __GFP_ACCOUNT and would it be correct to assume memdup_user() should use it unconditionally?
Otherwise my simple option would be to implement the logic in the set_description, but the benefit would be very local.
Please let me know what you think is best, happy to read more doc if there's a more productive way to address that
> > > + if (IS_ERR(d)) > > + return PTR_ERR(d); > > + > > + spin_lock(&file->f_lock); > > + kfree(file->f_description); > > + file->f_description = d; > > + spin_unlock(&file->f_lock); > > Generally kfree under spinlock is not good idea. > You can replace the pointer and free without spinlock.
Sorry I also need some pointers here - do you suggest I move the kfree out of the locked section or that there is a safe way other than spinlock?
> struct file is nicely aligned to 256 bytes on distro configs. > Will this break everything? > > $ cat /sys/kernel/slab/filp/object_size
Indeed on the config I'm using here this jumped to 264 bytes
Would it be better to move this to the inode struct? I don't know the implications of this - any other option?
Thanks!
|  |