Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Stephen Brennan <stephen.s.brennan@oracle.com>
To:     Jan Kara <jack@suse.cz>
Cc:     linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        Al Viro <viro@zeniv.linux.org.uk>,
        Stephen Brennan <stephen.s.brennan@oracle.com>,
        Amir Goldstein <amir73il@gmail.com>
Subject: [PATCH v4 5/5] fsnotify: require inode lock held during child flag update
Date:   Fri, 11 Nov 2022 14:06:14 -0800
Message-Id: <20221111220614.991928-6-stephen.s.brennan@oracle.com>
In-Reply-To: <20221111220614.991928-1-stephen.s.brennan@oracle.com>
References: <20221028001016.332663-1-stephen.s.brennan@oracle.com>
 <20221111220614.991928-1-stephen.s.brennan@oracle.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?oIW4ZQQEQSPatLs+VOyNFcZ7q34Ozc7tqOu9ZmRlyYe//OKlNS4F4goUc6k4?=
 =?us-ascii?Q?ERoqrZNkimPnb01ICpzwdVMxKBRJbMH5G1we9qyugJtNuyq8vFI6Uyv9zJel?=
 =?us-ascii?Q?vnvSCDBXjr1gVBAaETgmDlpZDVsEtaGzapAd19PmlMQ8XNpumrZZQVS3giMx?=
 =?us-ascii?Q?zIq8JwiMdSYS84xvWgkCpfo86/zUZxf9d87vUgR+yjkf42KDDNe+a9Ba9vCZ?=
 =?us-ascii?Q?bW44FchG8omei0Fm9uCiMzh1MYkuJOvHMCKZbYuazssw/8zd/NNaTkicfgMw?=
 =?us-ascii?Q?EWSq54B4c6lGMNNaznxCw4bo/xagQ5JDiOh5Rggjrct8LVnz+w19FQqwV2NR?=
 =?us-ascii?Q?UXURsZ6vzoPjTCa9CVY4qy9CB/+cvD6ugv+Wzznn042L66FYYFnSpWNJfCUQ?=
 =?us-ascii?Q?QPePVAMUwBubIu8UyIqVpR8icaDW7gsmH2rVpeUoxMnlHs2/fVJpWg5n0fJq?=
 =?us-ascii?Q?C3z5gLpSFyZ7QaTWXz3OdL0qiPJzxGKvhmpPSDHUCTPY/s+h4gKfPUIP3pSH?=
 =?us-ascii?Q?1sYtDPKdf+1RbaS3AomlB1CBZi8au4BMCTH/i84QzpLl4L1xkmHVnq3NFW+V?=
 =?us-ascii?Q?HCbBWr7dr4LeeDjgKeIBeoalTP9U3n5kw51bTXxIJREGqjGpdx0OMlpgXt4d?=
 =?us-ascii?Q?dqPPs4OdyzLY17Um17QrZ3NAni/X1VcQcwIIzXbsKeg5WNdScZSA4Hx0usBN?=
 =?us-ascii?Q?d3TS3dLeljv43yf3NfMV6vKxhsHTLPpLTNiuTTkEfqkj9R1PpjKzvlIVVcGS?=
 =?us-ascii?Q?YaQWl1B6Wi+ijOx5KH2YaNGz+Ywlj3AFHkYd+Zda4SL4YaiGu1PLxEZrnUAF?=
 =?us-ascii?Q?LCHQK+G9Lhio+1EtSUyZ6KOQlafKhk6DLyTILlPlfJZMeuj3nMj05ook3Efb?=
 =?us-ascii?Q?IqXzeBCuiMI689HDU5U2Cczm2OeXLYDXq9+YTJmJy5veqArumA5d8cszdIxg?=
 =?us-ascii?Q?ccus+eJWKwMKi7+YHh0q6moO+yxFZm5ClNO5xCn79ETO0YBiC3K7wHumfsRc?=
 =?us-ascii?Q?g2I/8hd5Kngogorkh29Qz70n/dvbXNZxV0BKSVYkRMCr9QpgayLDt6fujz9z?=
 =?us-ascii?Q?xPr8XN8Ch+hAgM+xxPWU7Kc7rZMEPdxUL+DWBqC838FoXB5oYqdv/+Sv5q93?=
 =?us-ascii?Q?5kckR2LwgZf3mMgklALtU+v75cjAbxOhoiDb7IvyClakazZ3yZRK4Re103nl?=
 =?us-ascii?Q?iYuPGTPz82oh0oFzEFZaBsG70YGpQbAlutyX3FDz2n0kuqRAqr1vkyUAapRU?=
 =?us-ascii?Q?eUxyT04lpNmH8F1wbeaPbQN03G8VZTdgJaItwRS9Ylch3lQtOP9TT8Kl06o1?=
 =?us-ascii?Q?9uxNkHzc/SXZMg+/K78aB/Hrzvhb6snc0dYZPzLMAayDq12iZi+r78Tv61Xp?=
 =?us-ascii?Q?lfwQT8cZIP7BzDz978mLObrIdNmPbwcBBaJcWJHxjYt8yKGRutv4EpPLrtk+?=
 =?us-ascii?Q?Rs27yyORqLIMIrUsc+lXQzlTCwpu7GP6LIEulaHezucjYD568ASVaPRSP4kY?=
 =?us-ascii?Q?LltWIqk2BFzo/1sk8mjLw+AHed2zNcB5XYSJFMh7o4bIYNCXsNGkHJ/f82vA?=
 =?us-ascii?Q?8EqnBz0XlbN8zR7O8i4VfWAZ/0ZZSHuu18njV86PLYnnpFHSZsk+cw8OAm2D?=
 =?us-ascii?Q?gQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: ERR566wigci8SddhBxDTim2rClU2xoyOMjdAVE3Ap4iBYsax0qNbKKLaZ+KKoFwGCtJDDZoXXOMt97ol4atXxESdVdGyTs+QonZ9lnC5N74ZnFTWh3XdBh6yObN6TCCI5X7U/Y3+34hbAsR+aC2BxeMywZzNHUVYmm0MAJLxDEdMQa/7mh754AocSaLLeSbg9dXElVAyAQ3ptrKlz6kGKb4j8l+P9uyvYvuuDGWHG1UC4S6lq1O0P2qV98MqB0OTOYOV7IWik4RoonW1YAQgmc3W3AC2Q/UrH24604OSJNtjSIlH3U1q2hmb36gZgDYKRcI6OKyacL7pi0O89+Y5u1yezlG5Hks5PG2AwbazBBLt9HYqHnRurscYvOS37bD7rfUFm2za4xjsr6L0fswB7oJKkjk5K8jFTIjzapUfrG7logBFHxu45DykQGGmBIdmxDJuBFA15ELXgrnLfQ8xbD3V4i+VQz4VP2jJNiQcVURpkqQD7cIfNDe9qd3suNt/h+b9/AFzACO16mZdF7s898c4Nk+9HDXe+87DVltqtZulxqsiDsyEuemP+/DvqRPA0QEYt/SFEhEJqEdMIYGSIk2H8hzFYg6SyGLbs++EQ2jv85uVfupnwKwO8MzrzuhkeNgWKghPHnIxBYaLdf8Jpa1FmXfzZc7h9W/2t2qNI9xWhxSSGdxSyaK6ufYFgq50847Jjgzum3tTxAqttvHhpDphXenGfHwPo3d/A5HhLMmr56Nkdfy6xvoMAKRm96EJlKiPhSdhu7q7jbtA/HHRuNUNLrHKyUAs5+LKKjiF8/I0P8eXhmDwZaNqzz2yZ9TksOnoPDlw5Ss2cnIyMPBxtfnDXjhN+hRwFBlf/IMrDV6R6tDEECFpRS4+xss7dtD66ogvq0o1MHfL/kXiSvNNbQ==
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 544f64f8-738e-41b5-e518-08dac430fad4
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4166.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Nov 2022 22:06:27.5215
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /gaz0czNXxvtwQfP58V1BKZ3Syo5lNGrGBNSr9xo/TEDYf8lAZ3wb12bsA07HGYwFWY5galK9zC4r2DKnNoNgd589FPR6sqimncAUmqfn4U=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR10MB4535
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.219,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-11-11_11,2022-11-11_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 adultscore=0
 malwarescore=0 mlxlogscore=999 mlxscore=0 phishscore=0 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2210170000
 definitions=main-2211110149
X-Proofpoint-GUID: iqMfUkqRJA6nKGlwYvPQ58NwXUvJFJnh
X-Proofpoint-ORIG-GUID: iqMfUkqRJA6nKGlwYvPQ58NwXUvJFJnh
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

With the prior changes to fsnotify, it is now possible for
fsnotify_recalc_mask() to return before all children flags have been
set. Imagine that two CPUs attempt to add a mark to an inode which would
require watching the children of that directory:

CPU 1:                                 CPU 2:

fsnotify_recalc_mask() {
  spin_lock();
  update_children = ...
  __fsnotify_recalc_mask();
  update_children = ...
  spin_unlock();
  // update_children is true!
  fsnotify_conn_set_children_dentry_flags() {
    // updating flags ...
    cond_resched();

                                       fsnotify_recalc_mask() {
                                         spin_lock();
                                         update_children = ...
                                         __fsnotify_recalc_mask();
                                         update_children = ...
                                         spin_unlock();
                                         // update_children is false
                                       }
                                       // returns to userspace, but
                                       // not all children are marked
    // continue updating flags
   }
}

To prevent this situation, hold the directory inode lock. This ensures
that any concurrent update to the mask will block until the update is
complete, so that we can guarantee that child flags are set prior to
returning.

Since the directory inode lock is now held during iteration over
d_subdirs, we are guaranteed that __d_move() cannot remove the dentry we
hold, so we no longer need check whether we should retry iteration. We
also are guaranteed that no cursors are moving through the list, since
simple_readdir() holds the inode read lock. Simplify the iteration by
removing this logic.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 fs/notify/fsnotify.c | 25 +++++++++----------------
 fs/notify/mark.c     |  8 ++++++++
 2 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 0ba61211456c..b5778775b88d 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -102,6 +102,8 @@ void fsnotify_sb_delete(struct super_block *sb)
  * on a child we run all of our children and set a dentry flag saying that the
  * parent cares.  Thus when an event happens on a child it can quickly tell
  * if there is a need to find a parent and send the event to the parent.
+ *
+ * Context: inode locked exclusive
  */
 void fsnotify_update_children_dentry_flags(struct inode *inode, bool watched)
 {
@@ -124,22 +126,16 @@ void fsnotify_update_children_dentry_flags(struct inode *inode, bool watched)
 	 * over d_subdirs which will allow us to sleep.
 	 */
 	spin_lock(&alias->d_lock);
-retry:
 	list_for_each_entry(child, &alias->d_subdirs, d_child) {
 		/*
-		 * We need to hold a reference while we sleep. But we cannot
-		 * sleep holding a reference to a cursor, or we risk skipping
-		 * through the list.
-		 *
-		 * When we wake, dput() could free the dentry, invalidating the
-		 * list pointers.  We can't look at the list pointers until we
-		 * re-lock the parent, and we can't dput() once we have the
-		 * parent locked.  So the solution is to hold onto our reference
-		 * and free it the *next* time we drop alias->d_lock: either at
-		 * the end of the function, or at the time of the next sleep.
+		 * We need to hold a reference while we sleep. When we wake,
+		 * dput() could free the dentry, invalidating the list pointers.
+		 * We can't look at the list pointers until we re-lock the
+		 * parent, and we can't dput() once we have the parent locked.
+		 * So the solution is to hold onto our reference and free it the
+		 * *next* time we drop alias->d_lock: either at the end of the
+		 * function, or at the time of the next sleep.
 		 */
-		if (child->d_flags & DCACHE_DENTRY_CURSOR)
-			continue;
 		if (need_resched()) {
 			dget(child);
 			spin_unlock(&alias->d_lock);
@@ -147,9 +143,6 @@ void fsnotify_update_children_dentry_flags(struct inode *inode, bool watched)
 			last_ref = child;
 			cond_resched();
 			spin_lock(&alias->d_lock);
-			/* Check for races with __d_move() */
-			if (child->d_parent != alias)
-				goto retry;
 		}
 
 		/*
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 6797a2952f87..f39cd88ad778 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -203,10 +203,15 @@ static void fsnotify_conn_set_children_dentry_flags(
 void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 {
 	bool update_children;
+	struct inode *inode = NULL;
 
 	if (!conn)
 		return;
 
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
+		inode = fsnotify_conn_inode(conn);
+		inode_lock(inode);
+	}
 	spin_lock(&conn->lock);
 	update_children = !fsnotify_conn_watches_children(conn);
 	__fsnotify_recalc_mask(conn);
@@ -219,6 +224,9 @@ void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	 */
 	if (update_children)
 		fsnotify_conn_set_children_dentry_flags(conn);
+
+	if (inode)
+		inode_unlock(inode);
 }
 
 /* Free all connectors queued for freeing once SRCU period ends */
-- 
2.34.1