Received-SPF: pass (google.com: domain of linux-kernel+bounces-67424-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Thu, 15 Feb 2024 12:40:45 -0500
From: Chuck Lever <chuck.lever@oracle.com>
To: "Liam R. Howlett" <Liam.Howlett@oracle.com>, Jan Kara <jack@suse.cz>,
        Chuck Lever <cel@kernel.org>, viro@zeniv.linux.org.uk,
        brauner@kernel.org, hughd@google.com, akpm@linux-foundation.org,
        oliver.sang@intel.com, feng.tang@intel.com,
        linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        maple-tree@lists.infradead.org, linux-mm@kvack.org, lkp@intel.com
Subject: Re: [PATCH RFC 7/7] libfs: Re-arrange locking in offset_iterate_dir()
Message-ID: <Zc5MnXdxASGiE3lK@tissot.1015granger.net>
References: <170785993027.11135.8830043889278631735.stgit@91.116.238.104.host.secureserver.net>
 <170786028847.11135.14775608389430603086.stgit@91.116.238.104.host.secureserver.net>
 <20240215131638.cxipaxanhidb3pev@quack3>
 <20240215170008.22eisfyzumn5pw3f@revolver>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240215170008.22eisfyzumn5pw3f@revolver>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?us-ascii?Q?odjVjMzcMmZ5hcngCRqVfTPbB2Bi8CWIdJfIjXjv6ibG47AKk2ho8GwmnYZ4?=
 =?us-ascii?Q?f3fKDes+miJEBVLcUrb0fqU1/hN8MVi3MJXBluEwQYBFDM2AYh/lbyclE5+R?=
 =?us-ascii?Q?oQ/oqyFHo30pCfjKFym6wix3kMgsF6c3aKISyULSkHTSBa3J/35fN769XO1S?=
 =?us-ascii?Q?5NQ5IKL69c1BuminDtSks6l5ffEaWU/Ccqr9Ay0fi1l/K1GUqvS4ZoAI7MRq?=
 =?us-ascii?Q?bSHrvz9WUqvIfuM68eakuut8uFlNxKobNUPcxhS7oVmsEZAHR4o8Kr14So22?=
 =?us-ascii?Q?0/hOev3OL9J4mJ4DjfzZH5L0E4x2y9zi3oAHZAo1cVkI9dfz+tmdERTCEiNy?=
 =?us-ascii?Q?6oGyH90HIrXhO1d96ttNNyvmUS9lj1QkPVx5p37dSQ6fn1cln9km00wmm7U6?=
 =?us-ascii?Q?7EZCzyNJKUdUCbRxXi9j/tLS3BkNW/RfZ9OLn6Qr+utXBiSIZ5IniTojxLSb?=
 =?us-ascii?Q?45DB9oUEIWdOnjmeYN6gdcHMT4XXZboy7CeKuhNLV/YExJhal1Pic92Iv9vg?=
 =?us-ascii?Q?qzXWnUG5hBhpZmakNZ7zBFFPLx7VGwNV3vUQGHqyvy9eNqYBgxFQmyk35KEF?=
 =?us-ascii?Q?lijkmMlPPguPD9fqx6QOe6e0+eVUGQOttJJvjjM4kKIx32JOG3dtV0YlVbWQ?=
 =?us-ascii?Q?k3OPVroVElxEzruIpxpsT0eQTHA2n4bI1jUsud9RQDwi+sKGsWGKY434Mf8i?=
 =?us-ascii?Q?pOVPDW6wd9kgXsjKdNLoPZ/JKTk870ftpqrXkKtpZCpp9BAxCIDpqS5VO78Y?=
 =?us-ascii?Q?4nJ9gYMRD/mF9kNqH1yn0QYiSrNqVoMELxaXbqof2qFopptlxNHAHR4f+2b4?=
 =?us-ascii?Q?Jh8ZXSvaPBzHCCMWNn6OaBGR3+8v9VZl/gTUdXQHa5a35aGJhyTohNVQPf2c?=
 =?us-ascii?Q?aLR4iQ1xJQ9CCca64yzZC3ygEedqPnwEQKVhQbRwE+PFHS0pLFxB7OTpjRah?=
 =?us-ascii?Q?enzJg/zL0GpNZOU9KDQr8H8Uc+U0V1wE1NBLi0DY9iz3+yFBgIbGH+sY3QFD?=
 =?us-ascii?Q?Wu51EfmPKHaFGcBqm6vDLSz25D13LP+OUb7SrN0OCUR8AkoVxxCAaYBt/LwC?=
 =?us-ascii?Q?MMhwvg6hzsgDdkzcFo1C0LkOzbAzmyan3AXI09bTFbvN2PV4SEgCeUyejeii?=
 =?us-ascii?Q?4y5KLxpRL61Ox7wVSASxj+/ec30xOM3N2ioI4nsUpgMb0EoTJve/cvbTWyzp?=
 =?us-ascii?Q?P9hU06N9S7+EGbeW86emTqxp1RM2dmeF66cgac8v6t5NXgLawI3tYnIaZu9e?=
 =?us-ascii?Q?LrawxQGAeXmvkg7hhiA2PFTFbMC81cPIZet36ecTlyomYaqT6/+bomnU2TzZ?=
 =?us-ascii?Q?usBL9z1YSJ6Ff3e4tz9T7o45A23u5+8kXPeKWItRgJ3DeXOIenLH5Er0u/0L?=
 =?us-ascii?Q?T9IJd+N949sqvRaYdHGhAfXMrdKt7RT6mF8D4Y+gDo5KLgyiN1HEGU6YnusS?=
 =?us-ascii?Q?XCQdm4cKIN+QXJahAm+3szdBHDWyP/CpuD/tEvZhQAC0M6UV7e7aeN2YP+Ak?=
 =?us-ascii?Q?sh9Is8JfHAK2dLjTeXppqyr102L9jMGtZCYjA91965L1L1ZsewmG0Dp44lFt?=
 =?us-ascii?Q?ugbcSJTAeI+pOzr6kJyjiuQYHoRy791It+i5g4sDL50kY2+jO1pC3my2If2z?=
 =?us-ascii?Q?VQ=3D=3D?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	zLIj32fh7nUXwIF0t+WwhXrkCEorCKqjvMlfRyFoqstFxik42No5at3mgpc1bHrsKBxtsg+xqUQZv+DangX/2NDjG6yCSsytKozmpEV4jGJQIYvvJJbgkmfxXZkQuZDZ6MjmXtv2GxCka7/x4BhhjLOdbiqahhkzH3Vg/YVTRqs66x7ewmYVziv/Gmbo7YY6f9GpCu+m29mY3cPlaxGUZahydvExhs0sI1sxI4huSrBTeqvJwPgH1hPVTrjntVQ40PtSvci1fZrhJacWA8kMadGuoTT1CHy179HZa+3Jsu/Co8y8ZgF2RL9eFmkmr8+YkvYJ7UJHEkxf2rvJL5OE6Tr2b8/+GZ14TyiZiEU8bZp4FUEA0uE2Xn+7f5470WBUyHFAR7v4Fcaz9H5Thabblgrj1NeQDYRTpyknRXEuLWLceYXbxwvEyNc5HI4XIHbELW1eQt6ndDINfSpjG3XKXEi/n+gHPn6EeMVkZp0Y9ttN6AwARJxJ/9CevsiL5Zw0kZrddWzAKSk99KMVvpXaCWNeUm03+q8KZH34PFTlV7FdeoabGqD8ZxJnFM3uBTuJ0QamTa0fAmrWj0xdjd77fLDX5sx6GkgS5AC+wYBf/nQ=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f10bf4d2-f73a-41d9-d133-08dc2e4d3f20
X-MS-Exchange-CrossTenant-AuthSource: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Feb 2024 17:40:48.7361
 (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: mODeaKH3jL19JOkA5JI6hstugqmGJpHYS/XyJ101ZN/78loe9qCnNDJw7cwfNP+3Qh3oT3OarebV73rBFcDQbw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BLAPR10MB5347
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-15_16,2024-02-14_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 mlxscore=0 spamscore=0
 adultscore=0 phishscore=0 bulkscore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2311290000
 definitions=main-2402150142
X-Proofpoint-GUID: PvIvs11Hhj82DLe_6KB-3r843ovobAXz
X-Proofpoint-ORIG-GUID: PvIvs11Hhj82DLe_6KB-3r843ovobAXz

On Thu, Feb 15, 2024 at 12:00:08PM -0500, Liam R. Howlett wrote:
> * Jan Kara <jack@suse.cz> [240215 08:16]:
> > On Tue 13-02-24 16:38:08, Chuck Lever wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > Liam says that, unlike with xarray, once the RCU read lock is
> > > released ma_state is not safe to re-use for the next mas_find() call.
> > > But the RCU read lock has to be released on each loop iteration so
> > > that dput() can be called safely.
> > > 
> > > Thus we are forced to walk the offset tree with fresh state for each
> > > directory entry. mt_find() can do this for us, though it might be a
> > > little less efficient than maintaining ma_state locally.
> > > 
> > > Since offset_iterate_dir() doesn't build ma_state locally any more,
> > > there's no longer a strong need for offset_find_next(). Clean up by
> > > rolling these two helpers together.
> > > 
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > 
> > Well, in general I think even xas_next_entry() is not safe to use how
> > offset_find_next() was using it. Once you drop rcu_read_lock(),
> > xas->xa_node could go stale. But since you're holding inode->i_rwsem when
> > using offset_find_next() you should be protected from concurrent
> > modifications of the mapping (whatever the underlying data structure is) -
> > that's what makes xas_next_entry() safe AFAIU. Isn't that enough for the
> > maple tree? Am I missing something?
> 
> If you are stopping, you should be pausing the iteration.  Although this
> works today, it's not how it should be used because if we make changes
> (ie: compaction requires movement of data), then you may end up with a
> UAF issue.  We'd have no way of knowing you are depending on the tree
> structure to remain consistent.
> 
> IOW the inode->i_rwsem is protecting writes of data but not the
> structure holding the data.
> 
> This is true for both xarray and maple tree.

Would it be appropriate to reorder this series so 7/7 comes before
the transition to use Maple Tree?


-- 
Chuck Lever