On Mon 12-05-25 11:52:14, Christian Brauner wrote: > On Tue, Apr 29, 2025 at 01:28:49PM +0200, Max Kellermann wrote: > > On Tue, Apr 29, 2025 at 1:12 PM Christian Brauner <brauner@xxxxxxxxxx> wrote: > > > > --- a/fs/read_write.c > > > > +++ b/fs/read_write.c > > > > @@ -332,7 +332,9 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence) > > > > struct inode *inode = file_inode(file); > > > > loff_t retval; > > > > > > > > - inode_lock(inode); > > > > + retval = inode_lock_killable(inode); > > > > > > That change doesn't seem so obviously fine to me. > > > > Why do you think so? And how is this different than the other two. > > chown_common() and chmod_common() are very close to the syscall boundary > so it's very unlikely that we run into weird issues apart from userspace > regression when they suddenly fail a change for new unexpected reasons. > > But just look at default_llseek(): > > > git grep default_llseek | wc -l > 461 > > That is a lot of stuff and it's not immediately clear how deeply or > nested they are called. For example from overlayfs in stacked > callchains. Who knows what strange assumptions some of the callers have > including the possible return values from that helper. > > > > > > Either way I'd like to see this split in three patches and some > > > reasoning why it's safe and some justification why it's wanted... > > > > Sure I can split this patch, but before I spend the time, I'd like us > > first to agree that the patch is useful. > > This is difficult to answer. Yes, on the face of it it seems useful to > be able to kill various operations that sleep on inode lock but who > knows what implicit guarantees/expectations we're going to break if we > do it. Maybe @Jan has some thoughts here as well. So I think having lock killable is useful and generally this should be safe wrt userspace (the process will get killed before it can notice the difference anyway). The concern about guarantees / expectations is still valid for the *kernel* code (which is I think what you meant above). So I guess some audit of kernel paths that can end up calling ->llseek handler and whether they are OK with the handler returning EINTR is needed. I expect this will be fine but I would not bet too much on it :) Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
