On Tue, Jan 7, 2020 at 7:13 PM Al Viro viro@zeniv.linux.org.uk wrote:
FWIW, I suspect that we want to do something along the following lines:
- make build_open_flags() treat O_CREAT | O_EXCL as if there had been
O_NOFOLLOW in the mix.
My reaction to that is "Whee, that's a big change".
But:
Benefit: this fragment in do_last()
you're right.
That's the semantics we have right now (and I think it's the correct safe semantics when I think about it). But when I first looked at your email I without thinking more about it actually thought we followed the symlink, and then did the O_CREAT | O_EXCL on the target (and potentially succeeded).
So I agree - making O_CREAT | O_EXCL imply O_NOFOLLOW seems to be the right thing to do, and not only should simplify our code, it's much more descriptive of what the real semantics are.
Even if my first reaction was that it would act differently.
Slash-and-burn approach to your explanatory subsequent steps:
- make follow_managed() take &inode and &seq.
- have the followup to failing __follow_mount_rcu() taken into it.
- fold __follow_mount_rcu() into follow_managed(), using the latter both in
RCU and non-RCU cases. 5) take the calls of follow_managed() out of lookup_fast() into its callers. 6) after that we have 3 callers of step_into(); [..] So if we manage to convert the damn thing in mountpoint_last() into follow_managed(), we could fold follow_managed() into step_into().
I think that all makes sense. I didn't go to look at the source, but from the email contents your steps look reasonable to me.
Another interesting question is whether we want O_PATH open to trigger automounts.
It does sound like they shouldn't, but as you say:
The thing is, we do *NOT* trigger them(or traverse mountpoints) at the starting point of lookups. I believe it's a mistake (and mine, at that), but I doubt that there's anything that can be done about it at that point. It's a user-visible behaviour [..]
Hmm. I wonder how set in stone that is. We may have two decades of history of not doing it at start point of lookups, but we do *not* have two decades of history of O_PATH.
So what I think we agree would be sane behavior would be for O_PATH opens to not trigger automounts (unless there's a slash at the end, whatever), but _do_ add the mount-point traversal to the beginning of lookups.
But only do it for the actual O_PATH fd case, not the cwd/root/non-O_PATH case.
That way we maintain original behavior: if somebody overmounts your cwd, you still see the pre-mount directory on lookups, because your cwd is "under" the mount.
But if you open a file with O_PATH, and somebody does a mount _afterwards_, the openat() will see that later mount and/or do the automount.
Don't you think that would be the more sane/obvious semantics of how O_PATH should work?
I think the easiest way to handle that is to have O_PATH turn LOOKUP_AUTOMOUNT, same as the normal open() does. That's trivial to do, but that changes user-visible behaviour. OTOH, with the current behaviour nobody can rely upon automount not getting triggered by somebody else just as they are entering their open(dir, O_PATH), so I think that's not a problem.
Linus, do you have any objections to such O_PATH semantics change?
See above: I think I'd prefer the O_PATH behavior the other way around. That seems to be more of a consistent behavior of what "O_PATH" means - it means "don't really open, we'll do it only when you use it as a directory".
But I don't have any _strong_ opinions. If you have a good reason to tell me that I'm being stupid, go ahead and do so and override my stupidity.
Linus