From patchwork Fri Jan 29 21:30:16 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Konstantin Khlebnikov X-Patchwork-Id: 8167631 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 00162BEEE5 for ; Fri, 29 Jan 2016 21:30:23 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id CE42620221 for ; Fri, 29 Jan 2016 21:30:22 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B39CF20154 for ; Fri, 29 Jan 2016 21:30:21 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753833AbcA2VaU (ORCPT ); Fri, 29 Jan 2016 16:30:20 -0500 Received: from mail-lf0-f43.google.com ([209.85.215.43]:34621 "EHLO mail-lf0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753049AbcA2VaS (ORCPT ); Fri, 29 Jan 2016 16:30:18 -0500 Received: by mail-lf0-f43.google.com with SMTP id 17so54943933lfz.1; Fri, 29 Jan 2016 13:30:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=Zo26mQJ0fFf/seo09d/YUgN1wR5yfcJSgPq/ddoJ/9g=; b=s0OwY41ew2WeaLN8J9s5/MlLI1Ztig73ttyhtvJ8IWM9evXH8Ns6f5nUX1MbEfqcxl jHcLRpA5+7tlizo/bb2O+krPzCfPVAe/DjDTbAq0iXHL9IK7s0z/hO/c7+oc7KreY3xl ekS/Hj47hDxkhnxSyPR6wk+aA9IQ4OvlZUQLVyX6/AvTVATJ1WNaZRAxMIKimWMl2hel F8rKgFDSBil3rPWvNONxnLGMyKSSzfCagc+Z2MBp1OUbmi443G2dycKorn3waK8OgCWm xo/La5IiBvEjf0aCzb2HMToDlGZg+TFLpcypMOs1+qyaDJzv8yeLg9sQljrAC3hpl/en uydQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=Zo26mQJ0fFf/seo09d/YUgN1wR5yfcJSgPq/ddoJ/9g=; b=gjjhLntBUJT+qgvgNrkdCA/zSpKiMBI30u3bo6e+jYKchgg3z0V9/0tLx5cD6rkJGs 8aiyQI/SJ3NhyV6I7ugKd51eOehjslszChx7IOECcMD5oDVaf2PI6p/BFaM+3OenqFq7 45X11gUNg8+YrWwLK1bLD7heXfIs2RqN+LYqhTo3yi7V6t5G7burXyDsw2JfyJztdyzG BgqaGLOgiewOiQb8PL0u1h/FDm0exdiNeuFpRhYW01bvOkCH73bZT/xzR32MIri7AwLh xbot3hKCxtbdzya7+r+m94jQrH0FjwrDtifq8zUkpI9neDTefxH8V/R4D0E31hIBb+ha 9h7w== X-Gm-Message-State: AG10YOTXnm03kPIPgQg+ZQC6F95/GsAvSOdrgEzub60or9DFEPIvQaz4hdxoyihh4vLleNygBvlnEgHCqNGmqg== MIME-Version: 1.0 X-Received: by 10.25.91.81 with SMTP id p78mr4264645lfb.19.1454103016496; Fri, 29 Jan 2016 13:30:16 -0800 (PST) Received: by 10.112.181.234 with HTTP; Fri, 29 Jan 2016 13:30:16 -0800 (PST) In-Reply-To: References: <20160125215832.GB8948@redhat.com> Date: Sat, 30 Jan 2016 00:30:16 +0300 Message-ID: Subject: Re: [PATCH] overlay: Clear stale ->numlower state over rename operation From: Konstantin Khlebnikov To: Vivek Goyal Cc: linux-unionfs@vger.kernel.org, Miklos Szeredi , David Howells , linux-fsdevel Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_DKIM_INVALID, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Fri, Jan 29, 2016 at 11:26 PM, Konstantin Khlebnikov wrote: > On Tue, Jan 26, 2016 at 12:58 AM, Vivek Goyal wrote: >> Over rename, we don't seem to be doing anything about the ->numlower >> state of ovl_entry and we continue to retain that. Fact of the matter >> is that ->numlower is not valid any more and it can interfere with >> other operations like file removal. So reset that state. >> >> This fixes the issue reported here. >> >> https://bugzilla.kernel.org/show_bug.cgi?id=109611 >> >> A previous attempt to fix the issue was here. >> >> http://marc.info/?l=linux-fsdevel&m=145252052703010&w=2 >> >> This probably is better fix than previous one. >> >> Here are the details of the problem. Do following. >> >> $ mkdir upper lower work merged upper/dir/ >> $ touch lower/test >> $ sudo mount -t overlay overlay -olowerdir=lower,upperdir=upper,workdir=work >> merged >> $ mv merged/test merged/dir/ >> $ rm merged/dir/test >> $ ls -l merged/dir/ >> /usr/bin/ls: cannot access merged/dir/test: No such file or directory >> total 0 >> c????????? ? ? ? ? ? test >> >> Basic problem seems to be that once a file has been unlinked, a >> whiteout has been left behind which was not needed and hence it becomes >> visible. >> >> whiteout is visible because parent dir is of not type MERGE, hence >> od->is_real is set during ovl_dir_open(). And that means ovl_iterate() >> passes on iterate handling directly to underlying fs. Underlying fs does >> not know/filter whiteouts so it becomes visible to user. >> >> Why did we leave a whiteout to begin with when we should not have. >> ovl_do_remove() checks for OVL_TYPE_PURE_UPPER() and does not leave >> whiteout if file is pure upper. In this case file is not found to be >> pure upper hence whiteout is left. >> >> So why file was not PURE_UPPER in this case? I think because dentry is >> still carrying some leftover state which was valid before rename. For example, >> od->numlower was set to 1 as it was a lower file. After rename, this state >> is not valid anymore as there is no such file in lower. > > I've stumbled upon the same bug. > > I'm affraid this solution is racy: ovl_path_real() works without any locks. > __upperdentry could appear at any time, but lowerstack[0] cannot disapper. > > I thiink the only option is keeping 'purity' status in ovl_entry independently. > Or better call is 'haslower'. It could differ from numlower !=0 for file entries > with upper dentry. After copy_up and rename entrly will keep references to > lower dentry from old location but haslower will be copied from entry from > new location. Or just use 'opaque' for that. As I see it already means exactly what needed. This should be enough, untested though. > >> >> Signed-off-by: Vivek Goyal >> --- >> fs/overlayfs/dir.c | 13 +++++++++++++ >> fs/overlayfs/overlayfs.h | 1 + >> fs/overlayfs/super.c | 13 +++++++++++++ >> 3 files changed, 27 insertions(+) >> >> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c >> index 692ceda..a165213 100644 >> --- a/fs/overlayfs/dir.c >> +++ b/fs/overlayfs/dir.c >> @@ -909,6 +909,19 @@ static int ovl_rename2(struct inode *olddir, struct dentry *old, >> ovl_dentry_set_opaque(new, old_opaque); >> } >> >> + /* >> + * As file/dir is being renamed, ->numlower state is stale. It >> + * should be ok to set it to zero as at new location file will >> + * be either upper/pure_upper and numlower will be zero. In >> + * case of directory, if destination dir is present it has to be >> + * empty dir and rename should be overwriting that directory and >> + * that should make lower level direcotry invisible hence >> + * numlower=0 makes sense there too. >> + */ >> + ovl_free_lower(old); >> + if (!overwrite) >> + ovl_free_lower(new); >> + >> if (cleanup_whiteout) >> ovl_cleanup(old_upperdir->d_inode, newdentry); >> >> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h >> index e17154a..d75b6a0 100644 >> --- a/fs/overlayfs/overlayfs.h >> +++ b/fs/overlayfs/overlayfs.h >> @@ -151,6 +151,7 @@ bool ovl_dentry_is_opaque(struct dentry *dentry); >> void ovl_dentry_set_opaque(struct dentry *dentry, bool opaque); >> bool ovl_is_whiteout(struct dentry *dentry); >> void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry); >> +void ovl_free_lower(struct dentry *dentry); >> struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, >> unsigned int flags); >> struct file *ovl_path_open(struct path *path, int flags); >> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c >> index e38ee0f..31f9920 100644 >> --- a/fs/overlayfs/super.c >> +++ b/fs/overlayfs/super.c >> @@ -205,6 +205,19 @@ void ovl_dentry_set_opaque(struct dentry *dentry, bool opaque) >> oe->opaque = opaque; >> } >> >> +void ovl_free_lower(struct dentry *dentry) { >> + struct ovl_entry *oe = dentry->d_fsdata; >> + int i; >> + >> + if (!oe->numlower) >> + return; >> + >> + for (i = 0; i < oe->numlower; i++) >> + dput(oe->lowerstack[i].dentry); >> + >> + oe->numlower = 0; >> +} >> + >> void ovl_dentry_update(struct dentry *dentry, struct dentry *upperdentry) >> { >> struct ovl_entry *oe = dentry->d_fsdata; >> -- >> 2.1.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/fs/overlayfs/super.c +++ b/fs/overlayfs/super.c @@ -76,12 +76,10 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry) if (oe->__upperdentry) { type = __OVL_PATH_UPPER; - if (oe->numlower) { - if (S_ISDIR(dentry->d_inode->i_mode)) - type |= __OVL_PATH_MERGE; - } else if (!oe->opaque) { + if (S_ISDIR(dentry->d_inode->i_mode) && oe->numlower) + type |= __OVL_PATH_MERGE; + else if (!oe->opaque) type |= __OVL_PATH_PURE; - } } else { if (oe->numlower > 1) type |= __OVL_PATH_MERGE;