> Op 07-04-2026 11:00 CEST schreef Mateusz Guzik <[email protected]>: > > > On Wed, Apr 1, 2026 at 12:25 PM Jori Koolstra <[email protected]> wrote: > > > > > > > Op 01-04-2026 06:19 CEST schreef Mateusz Guzik <[email protected]>: > > > > > > > > > On Tue, Mar 31, 2026 at 07:19:58PM +0200, Jori Koolstra wrote: > > > > @@ -5286,7 +5290,25 @@ int filename_mkdirat(int dfd, struct filename > > > > *name, umode_t mode) > > > > lookup_flags |= LOOKUP_REVAL; > > > > goto retry; > > > > } > > > > + > > > > + if (!error && (flags & MKDIRAT_FD_NEED_FD)) { > > > > + struct path new_path = { .mnt = path.mnt, .dentry = dentry > > > > }; > > > > + error = FD_ADD(0, dentry_open(&new_path, O_DIRECTORY, > > > > current_cred())); > > > > + } > > > > + end_creating_path(&path, dentry); > > > > return error; > > > > > > > > > You can't do it like this. Should it turn out no fd can be allocated, > > > the entire thing is going to error out while keeping the newly created > > > directory behind. You need to allocate the fd first, then do the hard > > > work, and only then fd_install and or free the fd. The FD_ADD machinery > > > can probably still be used provided proper wrapping of the real new > > > mkdir. > > > > But isn't this exactly what happens in open(O_CREAT) too? Eventually we > > call > > error = dir_inode->i_op->create(idmap, dir_inode, dentry, > > mode, open_flag & O_EXCL); > > > > and only then do we assign and install the fd. AFAIK there is no cleanup > > happening there either if the FD_ADD step fails. You will just have a > > regular file and no descriptor. But I would have to test this to be sure. > > > > FD_ADD(how->flags, do_file_open(dfd, name, &op)) means fd itself will > be allocated upfront and only then file creation will happen and which > is what I'm saying is how it should be done. With your patch the > directory is created first and the possibly failing fd allocation > happens later.
Err, you're right. I understand what you mean now. That does need to be fixed. I misremembered how FD_ADD works. I'll get back to this in the weekend. > > > Trying to handle this in open() is a no-go. openat2 is rather > > > problematic. > > > > I don't think that is necessarily true. It turned out O_CREAT | O_DIRECTORY > > was bugged for a very long time. Christian Brauner fixed it eventually, and > > that combination now returns EINVAL. But I think there is nothing really > > stopping us from implementing that combination in the expected way, apart > > from whatever reasons there were for not allowing this in the first place, > > which I don't know about (maybe mixing semantics?) > > > > I am not saying it's impossible. I am saying mkdir was always a > separate codepath and in order to change that you would need to add a > branchfest to open. I don't see any reason to go that route. That's a fair point. But there's also upsides like Aleksa has mentioned. I'm not very opinionated on the matter, especially since I don't know why those paths were ever separated. Thanks, Jori.
