> On Jul 9, 2025, at 11:28 PM, Song Liu <songliubraving@xxxxxxxx> wrote: [...] >>>> It isn't clear to me that vfs_walk_ancestors() needs to return anything. >>>> All the communication happens through walk_cb() >>>> >>>> walk_cb() is called with a path, the data, and a "may_sleep" flag. >>>> If it needs to sleep but may_sleep is not set, it returns "-ECHILD" >>>> which causes the walk to restart and use refcounts. >>>> If it wants to stop, it returns 0. >>>> If it wants to continue, it returns 1. >>>> If it wants a reference to the path then it can use (new) >>>> vfs_legitimize_path() which might fail. >>>> If it wants a reference to the path and may_sleep is true, it can use >>>> path_get() which won't fail. >>>> >>>> When returning -ECHILD (either because of a need to sleep or because >>>> vfs_legitimize_path() fails), walk_cb() would reset_data(). >>> >>> This might actually work. >>> >>> My only concern is with vfs_legitimize_path. It is probably safer if >>> we only allow taking references with may_sleep==true, so that path_get >>> won’t fail. In this case, we will not need walk_cb() to call >>> vfs_legitimize_path. If the user want a reference, the walk_cb will >>> first return -ECHILD, and call path_get when may_sleep is true. >> >> What is your concern with vfs_legitimize_path() ?? >> >> I've since realised that always restarting in response to -ECHILD isn't >> necessary and isn't how normal path-walk works. Restarting might be >> needed, but the first response to -ECHILD is to try legitimize_path(). >> If that succeeds, then it is safe to sleep. >> So returning -ECHILD might just result in vfs_walk_ancestors() calling >> legitimize_path() and then calling walk_cb() again. Why not have >> walk_cb() do the vfs_legitimize_path() call (which will almost always >> succeed in practice). > > After reading the emails and the code more, I think I misunderstood > why we need to call vfs_legitimize_path(). The goal of “legitimize” > is to get a reference on @path, so a reference-less walk may not > need legitimize_path() at all. Do I get this right this time? > > However, I still have some concern with legitimize_path: it requires > m_seq and r_seq recorded at the beginning of the walk, do we want > to pass those to walk_cb()? IIUC, one of the reason we prefer a > callback based solution is that it doesn’t expose nameidata (or a > subset of it). Letting walk_cb to call legitimize_path appears to > defeat this benefit, no? > > > A separate question below. > > I still have some question about how vfs_walk_ancestors() and the > walk_cb() interact. Let’s look at the landlock use case: the user > (landlock) just want to look at each ancestor, but doesn’t need to > take any references. walk_cb() will check @path against @root, and > return 0 when @path is the same as @root. > > IIUC, in this case, we will record m_seq and r_seq at the beginning > of vfs_walk_ancestors(), and check them against mount_lock and > rename_lock at the end of the walk. (Maybe we also need to check > them at some points before the end of the walk?) If either seq > changed during the walk, we need to restart the walk, and take > reference on each step. Did I get this right so far? > > If the above is right, here are my questions about the > reference-less walk above: > > 1. Which function (vfs_walk_ancestors or walk_cb) will check m_seq > and r_seq? I think vfs_walk_ancestors should check them. > 2. When either seq changes, which function will call reset_data? > I think there are 3 options here: > 2.a: vfs_walk_ancestors calls reset_data, which will be another > callback function the caller passes to vfs_walk_ancestors. > 2.b: walk_cb will call reset_data(), but we need a mechanism to > tell walk_cb to do it, maybe a “restart” flag? > 2.c: Caller of vfs_walk_ancestors will call reset_data(). In > this case, vfs_walk_ancestors will return -ECHILD to its > caller. But I think this option is NACKed. > > I think the right solution is to have vfs_walk_ancestors check > m_seq and r_seq, and have walk_cb call reset_data. But this is > Different to the proposal above. > > Do my questions above make any sense? Or maybe I totally > misunderstood something? Hi Neil, Did my questions/comments above make sense? I am hoping we can agree on some design soon. Christian and Mickaël, Could you please also share your thoughts on this? Current requirements from BPF side is straightforward: we just need a mechanism to “walk up one level and hold reference”. So most of the requirement comes from LandLock side. Thanks, Song
