On Sat, Dec 9, 2023 at 1:35 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > On 12/9/2023 10:08 AM, Paul Moore wrote: > > On Fri, Dec 8, 2023 at 7:24 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > >> On 12/8/2023 3:32 PM, Paul Moore wrote: > >>> On Fri, Dec 8, 2023 at 6:21 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote: > >>>> On 12/8/2023 2:43 PM, Paul Moore wrote: > >>>>> On Thu, Dec 7, 2023 at 9:14 PM Munehisa Kamata <kamatam@xxxxxxxxxx> wrote: > >>>>>> On Tue, 2023-12-05 14:21:51 -0800, Paul Moore wrote: > >>>>> .. > >>>>> > >>>>>>> I think my thoughts are neatly summarized by Andrew's "yuk!" comment > >>>>>>> at the top. However, before we go too much further on this, can we > >>>>>>> get clarification that Casey was able to reproduce this on a stock > >>>>>>> upstream kernel? Last I read in the other thread Casey wasn't seeing > >>>>>>> this problem on Linux v6.5. > >>>>>>> > >>>>>>> However, for the moment I'm going to assume this is a real problem, is > >>>>>>> there some reason why the existing pid_revalidate() code is not being > >>>>>>> called in the bind mount case? From what I can see in the original > >>>>>>> problem report, the path walk seems to work okay when the file is > >>>>>>> accessed directly from /proc, but fails when done on the bind mount. > >>>>>>> Is there some problem with revalidating dentrys on bind mounts? > >>>>>> Hi Paul, > >>>>>> > >>>>>> https://lkml.kernel.org/linux-fsdevel/20090608201745.GO8633@xxxxxxxxxxxxxxxxxx/ > >>>>>> > >>>>>> After reading this thread, I have doubt about solving this in VFS. > >>>>>> Honestly, however, I'm not sure if it's entirely relevant today. > >>>>> Have you tried simply mounting proc a second time instead of using a bind mount? > >>>>> > >>>>> % mount -t proc non /new/location/for/proc > >>>>> > >>>>> I ask because from your description it appears that proc does the > >>>>> right thing with respect to revalidation, it only becomes an issue > >>>>> when accessing proc through a bind mount. Or did I misunderstand the > >>>>> problem? > >>>> It's not hard to make the problem go away by performing some simple > >>>> action. I was unable to reproduce the problem initially because I > >>>> checked the Smack label on the bind mounted proc entry before doing > >>>> the cat of it. The problem shows up if nothing happens to update the > >>>> inode. > >>> A good point. > >>> > >>> I'm kinda thinking we just leave things as-is, especially since the > >>> proposed fix isn't something anyone is really excited about. > >> "We have to compromise the performance of our sandboxing tool because of > >> a kernel bug that's known and for which a fix is available." > >> > >> If this were just a curiosity that wasn't affecting real development I > >> might agree. But we've got a real world problem, and I don't see ignoring > >> it as a good approach. I can't see maintainers of other LSMs thinking so > >> if this were interfering with their users. > > While the reproducer may be written for Smack, there are plenty of > > indications that this applies to all LSMs and my comments have taken > > that into account. > > > > If you're really that upset, try channeling that outrage into your > > editor and draft a patch for this that isn't awful. > > We could "just" wait for the lsm_set_self_attr() syscall to land, and > suggest that it be used instead of the buggy /proc interfaces. You didn't like the "as-is" approach before, I'm a little surprised with the change in heart in less than 24 hours ... > I would love to propose a patch that's less sucky, but have not come > up with one. My understanding of VFS internals isn't up to the task, > I fear. I'm not convinced we've properly identified the root cause of the problem, once we have it might be easier to identify a solution. -- paul-moore.com
