[cc Christian] On Tue, Aug 15, 2023 at 6:12 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote: > > On Mon, 14 Aug 2023 at 16:05, Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > overlayfs file open (ovl_maybe_lookup_lowerdata) and overlay file llseek > > take the ovl_inode_lock, without holding upper sb_writers. > > > > In case of nested lower overlay that uses same upper fs as this overlay, > > lockdep will warn about (possibly false positive) circular lock > > dependency when doing open/llseek of lower ovl file during copy up with > > our upper sb_writers held, because the locking ordering seems reverse to > > the locking order in ovl_copy_up_start(): > > > > - lower ovl_inode_lock > > - upper sb_writers > > > > Take upper sb_writers only when we actually need it, so we won't hold it > > during lower file open and lower file llseek to avoid the lockdep warning. > > > > Minimizing the scope of ovl_want_write() during copy up is also needed > > for fixing other possible deadlocks by following patches. > > > > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx> > > --- > > fs/overlayfs/copy_up.c | 117 +++++++++++++++++++++++++++++++---------- > > 1 file changed, 88 insertions(+), 29 deletions(-) > > > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c > > index c998dab440f8..f2a31ff790fb 100644 > > --- a/fs/overlayfs/copy_up.c > > +++ b/fs/overlayfs/copy_up.c > > @@ -251,8 +251,13 @@ static int ovl_copy_up_file(struct ovl_fs *ofs, struct dentry *dentry, > > if (IS_ERR(old_file)) > > return PTR_ERR(old_file); > > > > + error = ovl_want_write(dentry); > > + if (error) > > + goto out_fput; > > What occurs to me is why are we bothering with getting write access on > the internal upper mnt each time. Seems to me it's a historical thing > without a good reason. Upper mnt is never changed from R/W to R/O. > > So the only thing we need to do is grab the upper mount write access > on superblock creation and do the sb_start_write/end_write() thing > which can't fail. If upper mnt is read-only, we effectively have a > read-only filesystem, and can handle it that way (sb->s_flags |= > SB_RDONLY). > > There's still the possibility that we do some changes to upper even > for non-modify operations. But with careful review we can remove a > most (possibly all) error handling cases from ovl_want_write() > callsites when we do know that we have write access on upper. And > WARN_ON(__mnt_is_readonly(ovl_upper_mnt(ofs))) should ensure that we > catch any mistakes. > > Hmm? > I was thinking the same thing myself, before I went on this journey. I reached the conclusion that doing only sb_start_write() would not be safe against emergency remount rdonly of the upper sb. I guess if upper sb is emergency mounted rdonly, then overlayfs sb would also be emergency remounted rdonly, but for example ext4 sb can become rdonly on internal errors. But maybe that is not the responsibility of vfs or ovl to care about? Christian, is there also an API to set the sb rdonly when private writable mounts (i.e. ovl_upper_mnt) exist? Thanks, Amir.
