On Thu 08-02-24 16:04:29, Amir Goldstein wrote: > > > On Mon 29-01-24 20:30:34, Amir Goldstein wrote: > > > > On Mon, Dec 18, 2023 at 5:53 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > In the HttpDirFS HSM demo, I used FAN_OPEN_PERM on a mount mark > > > > > to deny open of file during the short time that it's content is being > > > > > punched out. > > > > > It is quite complicated to explain, but I only used it for denying access, > > > > > not to fill content and not to write anything to filesystem. > > > > > It's worth noting that returning EBUSY in that case would be more meaningful > > > > > to users. > > > > > > > > > > That's one case in favor of allowing FAN_DENY_ERRNO for FAN_OPEN_PERM, > > > > > but mainly I do not have a proof that people will not need it. > > > > > > > > > > OTOH, I am a bit concerned that this will encourage developer to use > > > > > FAN_OPEN_PERM as a trigger to filling file content and then we are back to > > > > > deadlock risk zone. > > > > > > > > > > Not sure which way to go. > > > > > > > > > > Anyway, I think we agree that there is no reason to merge FAN_DENY_ERRNO > > > > > before FAN_PRE_* events, so we can continue this discussion later when > > > > > I post FAN_PRE_* patches - not for this cycle. > > > > > > > > I started to prepare the pre-content events patches for posting and got back > > > > to this one as well. > > > > > > > > Since we had this discussion I have learned of another use case that > > > > requires filling file content in FAN_OPEN_PERM hook, FAN_OPEN_EXEC_PERM > > > > to be exact. > > > > > > > > The reason is that unless an executable content is filled at execve() time, > > > > there is no other opportunity to fill its content without getting -ETXTBSY. > > > > > > Yes, I've been scratching my head over this usecase for a few days. I was > > > thinking whether we could somehow fill in executable (and executed) files on > > > access but it all seemed too hacky so I agree that we probably have to fill > > > them in on open. > > > > > > > Normally, I think there will not be a really huge executable(?) > > If there were huge executables, they would have likely been broken down > > into smaller loadable libraries which should allow more granular > > content filling, > > but I guess there will always be worst case exceptions. > > > > > > So to keep things more flexible, I decided to add -ETXTBSY to the > > > > allowed errors with FAN_DENY_ERRNO() and to decided to allow > > > > FAN_DENY_ERRNO() with all permission events. > > > > > > > > To keep FAN_DENY_ERRNO() a bit more focused on HSM, I have > > > > added a limitation that FAN_DENY_ERRNO() is allowed only for > > > > FAN_CLASS_PRE_CONTENT groups. > > > > > > I have no problem with adding -ETXTBSY to the set of allowed errors. That > > > makes sense. Adding FAN_DENY_ERRNO() to all permission events in > > > FAN_CLASS_PRE_CONTENT groups - OK, > > > > done that. > > > > I am still not very happy about FAN_OPEN_PERM being part of HSM > > event family when I know that O_TRUCT and O_CREAT call this hook > > with sb writers held. > > > > The irony, is that there is no chance that O_TRUNC will require filling > > content, same if the file is actually being created by O_CREAT, so the > > cases where sb writers is actually needed and the case where content > > filling is needed do not overlap, but I cannot figure out how to get those > > cases out of the HSM risk zone. Ideas? > > > > Jan, > > I wanted to run an idea by you. > > I like your idea to start a clean slate with > FAN_CLASS_PRE_CONTENT | FAN_REPORT_FID > and it would be nice if we could restrict this HSM to use > pre-content events, which is why I was not happy about allowing > FAN_DENY_ERRNO() for the legacy FAN_OPEN*_PERM events, > especially with the known deadlocks. > > Since we already know that we need to generate > FAN_PRE_ACCESS(offset,length) for read-only mmap() and > FAN_PRE_MODIFY(offset,length) for writable mmap(), > we could treat uselib() and execve() the same way and generate > FAN_PRE_ACCESS(0,i_size) as if the file was mmaped. BTW uselib() is deprecated and there is a patch queued to not generate OPEN_EXEC events for it because it was causing problems (not the generation of events itself but the FMODE_EXEC bit being set in uselib). So I don't think we need to instrument uselib(). > My idea is to generate an event FAN_PRE_MODIFY(0,0) > for an open for write *after* file was truncated and > FAN_PRE_ACCESS(0,0) for open O_RDONLY. What I find somewhat strange about this is that if we return error from the fsnotify_file_perm() hook, open(2) will fail with error but the file is already truncated. But I guess it should be rare and it's bearable. > Possibly also FAN_PRE_*(offset,0) events for llseek(). That seem overdoing it a bit IMO :) > I've already pushed a POC to fan_pre_content branch [1]. > Only sanity tested that nothing else is broken. > I still need to add the mmap hooks and test the new events. > > With this, HSM will have appropriate hooks to fill executable > and library on first access and also fill arbitrary files on open > including the knowledge if the file was opened for write. > > Thoughts? Yeah, I guess this looks sensible. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
