Thanks for reviewing, I added some questions inline below On Mon, Jul 27, 2020 at 7:21 AM Alexey Dobriyan <adobriyan@xxxxxxxxx> 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!
