| Message ID | 20200217131455.31107-12-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fanotify event with name info | expand |
On Mon 17-02-20 15:14:50, Amir Goldstein wrote: > For some events, we are going to encode both child and parent fid's, > so we need to do a little refactoring of struct fanotify_event and fid > helper functions. > > Move fsid member from struct fanotify_fid out to struct fanotify_event, > so we can store fsid once for two encoded fid's (we will only encode > parent if it is on the same filesystem). > > This does not change the size of struct fanotify_event because struct > fanotify_fid is still bigger than struct path on 32bit arch and is the > same size as struct path (16 bytes) on 64bit arch. > > Group fh_len and fh_type as struct fanotify_fid_hdr. > Pass struct fanotify_fid and struct fanotify_fid_hdr to helpers > fanotify_encode_fid() and copy_fid_to_user() instead of passing the > containing fanotify_event struct. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> ... > @@ -327,16 +327,18 @@ init: __maybe_unused > event->pid = get_pid(task_pid(current)); > else > event->pid = get_pid(task_tgid(current)); > - event->fh_len = 0; > + event->fh.len = 0; > + if (fsid) > + event->fsid = *fsid; > if (id && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > /* Report the event without a file identifier on encode error */ > event->fh_type = fanotify_encode_fid(event, id, gfp, fsid); ^^^^ This should be event->fh, shouldn't it? I wonder how come 0-day didn't catch this... > +struct fanotify_fid_hdr { > + u8 type; > + u8 len; > +}; > + > struct fanotify_fid { > - __kernel_fsid_t fsid; > union { > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > unsigned char *ext_fh; > }; > }; ... > @@ -63,13 +81,13 @@ struct fanotify_event { > u32 mask; > /* > * Those fields are outside fanotify_fid to pack fanotify_event nicely > - * on 64bit arch and to use fh_type as an indication of whether path > + * on 64bit arch and to use fh.type as an indication of whether path > * or fid are used in the union: > * FILEID_ROOT (0) for path, > 0 for fid, FILEID_INVALID for neither. > */ > - u8 fh_type; > - u8 fh_len; > + struct fanotify_fid_hdr fh; > u16 pad; The 'pad' here now looks rather bogus. Let's remove it and leave padding on the compiler. It's in-memory struct anyway... > + __kernel_fsid_t fsid; > union { > /* > * We hold ref to this path so it may be dereferenced at any Here I disagree. IMO 'fsid' should be still part of the union below because the "object identification" is either struct path or (fsid + fh). I understand that you want to reuse fsid for the other file handle. But then I believe it should rather be done like: struct fanotify_fh { union { unsigned char fh[FANOTIFY_INLINE_FH_LEN]; unsigned char *ext_fh; }; }; struct fanotify_fid { __kernel_fsid_t fsid; struct fanotify_fh object; struct fanotify_fh dir; } BTW, is file handle length and type guaranteed to be the same for 'object' and 'dir'? Given how filehandles try to be rather opaque sequences of bytes, I'm not sure we are safe to assume that... Honza
On Wed, Feb 26, 2020 at 12:23 PM Jan Kara <jack@suse.cz> wrote: > > On Mon 17-02-20 15:14:50, Amir Goldstein wrote: > > For some events, we are going to encode both child and parent fid's, > > so we need to do a little refactoring of struct fanotify_event and fid > > helper functions. > > > > Move fsid member from struct fanotify_fid out to struct fanotify_event, > > so we can store fsid once for two encoded fid's (we will only encode > > parent if it is on the same filesystem). > > > > This does not change the size of struct fanotify_event because struct > > fanotify_fid is still bigger than struct path on 32bit arch and is the > > same size as struct path (16 bytes) on 64bit arch. > > > > Group fh_len and fh_type as struct fanotify_fid_hdr. > > Pass struct fanotify_fid and struct fanotify_fid_hdr to helpers > > fanotify_encode_fid() and copy_fid_to_user() instead of passing the > > containing fanotify_event struct. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > ... > > > @@ -327,16 +327,18 @@ init: __maybe_unused > > event->pid = get_pid(task_pid(current)); > > else > > event->pid = get_pid(task_tgid(current)); > > - event->fh_len = 0; > > + event->fh.len = 0; > > + if (fsid) > > + event->fsid = *fsid; > > if (id && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { > > /* Report the event without a file identifier on encode error */ > > event->fh_type = fanotify_encode_fid(event, id, gfp, fsid); > ^^^^ > This should be event->fh, shouldn't it? I wonder how come 0-day didn't > catch this... Maybe 0-day is on vacation... > > > +struct fanotify_fid_hdr { > > + u8 type; > > + u8 len; > > +}; > > + > > struct fanotify_fid { > > - __kernel_fsid_t fsid; > > union { > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > unsigned char *ext_fh; > > }; > > }; > ... > > @@ -63,13 +81,13 @@ struct fanotify_event { > > u32 mask; > > /* > > * Those fields are outside fanotify_fid to pack fanotify_event nicely > > - * on 64bit arch and to use fh_type as an indication of whether path > > + * on 64bit arch and to use fh.type as an indication of whether path > > * or fid are used in the union: > > * FILEID_ROOT (0) for path, > 0 for fid, FILEID_INVALID for neither. > > */ > > - u8 fh_type; > > - u8 fh_len; > > + struct fanotify_fid_hdr fh; > > u16 pad; > > The 'pad' here now looks rather bogus. Let's remove it and leave padding on > the compiler. It's in-memory struct anyway... ok. > > > + __kernel_fsid_t fsid; > > union { > > /* > > * We hold ref to this path so it may be dereferenced at any > > Here I disagree. IMO 'fsid' should be still part of the union below because > the "object identification" is either struct path or (fsid + fh). I > understand that you want to reuse fsid for the other file handle. But then > I believe it should rather be done like: > > struct fanotify_fh { > union { > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > unsigned char *ext_fh; > }; > }; > This I will do. > struct fanotify_fid { > __kernel_fsid_t fsid; > struct fanotify_fh object; > struct fanotify_fh dir; > } > object and dir do not end up in the same struct. object is in fanotify_event dir is in the extended fanotify_name_event, but I can do: struct fanotify_fid { __kernel_fsid_t fsid; struct fanotify_fh fh; } struct fanotify_event { struct fsnotify_event fse; u32 mask; struct fanotify_fid_hdr fh; struct fanotify_fid_hdr dfh; union { struct path path; struct fanotify_fid object; }; struct pid *pid; }; struct fanotify_name_event { struct fanotify_event fae; struct fanotify_fh dir; struct qstr name; unsigned char inline_name[FANOTIFY_INLINE_NAME_LEN]; }; > BTW, is file handle length and type guaranteed to be the same for 'object' and > 'dir'? Given how filehandles try to be rather opaque sequences of bytes, > I'm not sure we are safe to assume that... No and as you can see in the final struct above, we are not assuming that we are only using safe fsid. Looking at the final result, do you agree to leave fsid outside of struct fanotify_fid as I did, or do you still dislike it being outside of the union? Thanks, Amir.
On Wed 26-02-20 13:53:06, Amir Goldstein wrote: > On Wed, Feb 26, 2020 at 12:23 PM Jan Kara <jack@suse.cz> wrote: > > > > > + __kernel_fsid_t fsid; > > > union { > > > /* > > > * We hold ref to this path so it may be dereferenced at any > > > > Here I disagree. IMO 'fsid' should be still part of the union below because > > the "object identification" is either struct path or (fsid + fh). I > > understand that you want to reuse fsid for the other file handle. But then > > I believe it should rather be done like: > > > > struct fanotify_fh { > > union { > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > unsigned char *ext_fh; > > }; > > }; > > > > This I will do. > > > struct fanotify_fid { > > __kernel_fsid_t fsid; > > struct fanotify_fh object; > > struct fanotify_fh dir; > > } > > > > object and dir do not end up in the same struct. Right, ok. > object is in fanotify_event > dir is in the extended fanotify_name_event, but I can do: > > struct fanotify_fid { > __kernel_fsid_t fsid; > struct fanotify_fh fh; > } > > struct fanotify_event { > struct fsnotify_event fse; > u32 mask; > struct fanotify_fid_hdr fh; > struct fanotify_fid_hdr dfh; > union { > struct path path; > struct fanotify_fid object; > }; > struct pid *pid; > }; > > struct fanotify_name_event { > struct fanotify_event fae; > struct fanotify_fh dir; > struct qstr name; > unsigned char inline_name[FANOTIFY_INLINE_NAME_LEN]; > }; Looking at this I'm not quite happy either :-| E.g. 'dfh' contents here somewhat magically tells that this is not fanotify_event but fanotify_name_event. Also I agree that fsid hidden in 'object' is not ideal although I still dislike having it directly in fanotify_event as for path events it will not be filled and that can lead to confusion. I understand this is so convoluted because there are several constraints: 1) We don't want to grow event size unnecessarily. 2) We prefer allocating from dedicated slab cache 3) We have events of several types needing to store different kind of information. But seeing how things evolve I think we should consider relaxing some of the constraints to make the code easier to follow. How about having something like: struct fanotify_event { struct fsnotify_event fse; u32 mask; enum fanotify_event_type type; struct pid *pid; }; where type would identify what kind of event we have. Then we would have struct fanotify_path_event { struct fanotify_event fae; struct path path; }; struct fanotify_perm_path_event { struct fanotify_event fae; struct path path; unsigned short response; unsigned short state; int fd; }; struct fanotify_fh { u8 type; u8 len; union { unsigned char fh[FANOTIFY_INLINE_FH_LEN]; unsigned char *ext_fh; }; }; struct fanotify_fid_event { struct fanotify_event fae; __kernel_fsid_t fsid; struct fanotify_fh object_fh; }; struct fanofify_name_event { struct fanotify_event fae; __kernel_fsid_t fsid; struct fanotify_fh object_fh; struct fanotify_fh dir_fh; u8 name_len; char name[0]; }; WRT size, this would grow fanotify_fid_event by 1 long on 64-bits, fanotify_path_event would be actually smaller by 1 long, fanofify_name_event would be smaller but that's not really comparable because you chose a solution with fixed-inline length while I'd just go with allocating from kmalloc when we have to store the name. In terms of kmalloc caches, we would need three: for path, perm_path, fid events, I'd allocate name events from generic kmalloc caches. So overall I think this would be better. The question is whether the resulting code will really be more readable. I hope so because the structures are definitely nicer this way and things belonging logically together are now together. But you never know until you convert the code... Would you be willing to try this refactoring? Honza
On Wed, Feb 26, 2020 at 7:07 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 26-02-20 13:53:06, Amir Goldstein wrote: > > On Wed, Feb 26, 2020 at 12:23 PM Jan Kara <jack@suse.cz> wrote: > > > > > > > + __kernel_fsid_t fsid; > > > > union { > > > > /* > > > > * We hold ref to this path so it may be dereferenced at any > > > > > > Here I disagree. IMO 'fsid' should be still part of the union below because > > > the "object identification" is either struct path or (fsid + fh). I > > > understand that you want to reuse fsid for the other file handle. But then > > > I believe it should rather be done like: > > > > > > struct fanotify_fh { > > > union { > > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > > unsigned char *ext_fh; > > > }; > > > }; > > > > > > > This I will do. > > > > > struct fanotify_fid { > > > __kernel_fsid_t fsid; > > > struct fanotify_fh object; > > > struct fanotify_fh dir; > > > } > > > > > > > object and dir do not end up in the same struct. > > Right, ok. > > > object is in fanotify_event > > dir is in the extended fanotify_name_event, but I can do: > > > > struct fanotify_fid { > > __kernel_fsid_t fsid; > > struct fanotify_fh fh; > > } > > > > struct fanotify_event { > > struct fsnotify_event fse; > > u32 mask; > > struct fanotify_fid_hdr fh; > > struct fanotify_fid_hdr dfh; > > union { > > struct path path; > > struct fanotify_fid object; > > }; > > struct pid *pid; > > }; > > > > struct fanotify_name_event { > > struct fanotify_event fae; > > struct fanotify_fh dir; > > struct qstr name; > > unsigned char inline_name[FANOTIFY_INLINE_NAME_LEN]; > > }; > > Looking at this I'm not quite happy either :-| E.g. 'dfh' contents here > somewhat magically tells that this is not fanotify_event but > fanotify_name_event. Also I agree that fsid hidden in 'object' is not ideal > although I still dislike having it directly in fanotify_event as for path > events it will not be filled and that can lead to confusion. > > I understand this is so convoluted because there are several constraints: > 1) We don't want to grow event size unnecessarily. > 2) We prefer allocating from dedicated slab cache > 3) We have events of several types needing to store different kind of > information. > > But seeing how things evolve I think we should consider relaxing some of > the constraints to make the code easier to follow. How about having > something like: > > struct fanotify_event { > struct fsnotify_event fse; > u32 mask; > enum fanotify_event_type type; > struct pid *pid; > }; > > where type would identify what kind of event we have. Then we would have > > struct fanotify_path_event { > struct fanotify_event fae; > struct path path; > }; > > struct fanotify_perm_path_event { > struct fanotify_event fae; > struct path path; Any reason not to "inherit" from fanotify_path_event? There is code that is generic to permission and non-permission path events that accesses event->path and I wouldn't want to make that code two cases instead of just one. > unsigned short response; > unsigned short state; > int fd; > }; > > struct fanotify_fh { > u8 type; > u8 len; That's a 6 bytes hole! and then there are two of those in object_fh and dir_fh. That is why I stored the header in separate from the fh itself so that two headers could pack up nicely and yes, I also used the headers as an event type indication. > union { > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > unsigned char *ext_fh; > }; > }; > > struct fanotify_fid_event { > struct fanotify_event fae; > __kernel_fsid_t fsid; > struct fanotify_fh object_fh; > }; > > struct fanofify_name_event { > struct fanotify_event fae; > __kernel_fsid_t fsid; > struct fanotify_fh object_fh; Again, any reason not to "inherit" from fanotify_fid_event? There is plenty of code that is common to fid and name events because name events are also fid events. > struct fanotify_fh dir_fh; > u8 name_len; > char name[0]; > }; > > WRT size, this would grow fanotify_fid_event by 1 long on 64-bits, > fanotify_path_event would be actually smaller by 1 long, fanofify_name_event > would be smaller but that's not really comparable because you chose a > solution with fixed-inline length while I'd just go with allocating from > kmalloc when we have to store the name. OK. Same an inotify. I guess I started with the name_snapshot thing that was really fixed-size event and then reused the same construct without the snapshot, but I guess we can do away with the inline name. > > In terms of kmalloc caches, we would need three: for path, perm_path, fid > events, I'd allocate name events from generic kmalloc caches. > > So overall I think this would be better. The question is whether the > resulting code will really be more readable. I hope so because the > structures are definitely nicer this way and things belonging logically > together are now together. But you never know until you convert the code... > Would you be willing to try this refactoring? Yes, but I would like to know what you think about the two 6 byte holes Just let that space be wasted for the sake of nicer abstraction? It seems like too much to me. Thanks, Amir.
> > So overall I think this would be better. The question is whether the > > resulting code will really be more readable. I hope so because the > > structures are definitely nicer this way and things belonging logically > > together are now together. But you never know until you convert the code... > > Would you be willing to try this refactoring? > > Yes, but I would like to know what you think about the two 6 byte holes > Just let that space be wasted for the sake of nicer abstraction? > It seems like too much to me. > What if we unite the fh and name into one struct and keep a 32bit hash of fh+name inside? This will allow us to mitigate the cost of memcmp of fh+name in merge and get rid of objectid in fsnotify_event as you suggested. struct fanotify_fh_name { union { struct { u8 fh_type; u8 fh_len; u8 name_len; u32 hash; }; u64 hash_len; }; union { unsigned char fh[FANOTIFY_INLINE_FH_LEN]; unsigned char *ext_fh; }; char name[0]; }; struct fanotify_fid_event { struct fanotify_event fae; __kernel_fsid_t fsid; struct fanotify_fh_name object_fh; /* name is empty */ }; struct fanofify_name_event { struct fanotify_fid_event ffe; struct fanotify_fh_name dirent; }; So the only anomaly is that we use struct fanotify_fh_name to describe object_fh which never has a name. I think we can live with that and trying to beat that would be over abstraction. Thoughts? Thanks, Amir.
On Wed 26-02-20 19:50:30, Amir Goldstein wrote: > On Wed, Feb 26, 2020 at 7:07 PM Jan Kara <jack@suse.cz> wrote: > > Looking at this I'm not quite happy either :-| E.g. 'dfh' contents here > > somewhat magically tells that this is not fanotify_event but > > fanotify_name_event. Also I agree that fsid hidden in 'object' is not ideal > > although I still dislike having it directly in fanotify_event as for path > > events it will not be filled and that can lead to confusion. > > > > I understand this is so convoluted because there are several constraints: > > 1) We don't want to grow event size unnecessarily. > > 2) We prefer allocating from dedicated slab cache > > 3) We have events of several types needing to store different kind of > > information. > > > > But seeing how things evolve I think we should consider relaxing some of > > the constraints to make the code easier to follow. How about having > > something like: > > > > struct fanotify_event { > > struct fsnotify_event fse; > > u32 mask; > > enum fanotify_event_type type; > > struct pid *pid; > > }; > > > > where type would identify what kind of event we have. Then we would have > > > > struct fanotify_path_event { > > struct fanotify_event fae; > > struct path path; > > }; > > > > struct fanotify_perm_path_event { > > struct fanotify_event fae; > > struct path path; > > Any reason not to "inherit" from fanotify_path_event? > There is code that is generic to permission and non-permission path > events that accesses event->path and I wouldn't > want to make that code two cases instead of just one. I'm OK with that if it works better for you. I was just thinking that we'll have a helper like: struct path *fanotify_event_path(struct fanotify_event *event) { if (event->type == FA_PATH_EVENT) return ((struct fanotify_path_event *)event)->path; else if (event->type == FA_PERM_PATH_EVENT) return ((struct fanotify_perm_path_event *)event)->path; else return NULL; } and thus in most of code all the type details could be abstracted by this helper and so there won't be reason for "intermediate" types. But as I wrote above if you find good use for them, I'm OK with that. > > unsigned short response; > > unsigned short state; > > int fd; > > }; > > > > struct fanotify_fh { > > u8 type; > > u8 len; > > That's a 6 bytes hole! and then there are two of those > in object_fh and dir_fh. > That is why I stored the header in separate from the fh itself > so that two headers could pack up nicely and yes, > I also used the headers as an event type indication. Yes, I know but this packing of loosely related things is exactly what makes the code difficult to follow... > > union { > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > unsigned char *ext_fh; > > }; > > }; > > > > struct fanotify_fid_event { > > struct fanotify_event fae; > > __kernel_fsid_t fsid; > > struct fanotify_fh object_fh; > > }; > > > > struct fanofify_name_event { > > struct fanotify_event fae; > > __kernel_fsid_t fsid; > > struct fanotify_fh object_fh; > > Again, any reason not to "inherit" from fanotify_fid_event? > There is plenty of code that is common to fid and name events > because name events are also fid events. We could if the helper functions do not abstract the difference enough... > > struct fanotify_fh dir_fh; > > u8 name_len; > > char name[0]; > > }; > > > > WRT size, this would grow fanotify_fid_event by 1 long on 64-bits, > > fanotify_path_event would be actually smaller by 1 long, fanofify_name_event > > would be smaller but that's not really comparable because you chose a > > solution with fixed-inline length while I'd just go with allocating from > > kmalloc when we have to store the name. > > OK. Same an inotify. > I guess I started with the name_snapshot thing that was really fixed-size > event and then reused the same construct without the snapshot, but I > guess we can do away with the inline name. > > > In terms of kmalloc caches, we would need three: for path, perm_path, fid > > events, I'd allocate name events from generic kmalloc caches. > > > > So overall I think this would be better. The question is whether the > > resulting code will really be more readable. I hope so because the > > structures are definitely nicer this way and things belonging logically > > together are now together. But you never know until you convert the code... > > Would you be willing to try this refactoring? > > Yes, but I would like to know what you think about the two 6 byte holes > Just let that space be wasted for the sake of nicer abstraction? > It seems like too much to me. Well, it's wasting 1 long per FID event (i.e., 72 vs 64 bytes on 64-bits if I'm counting right) compared to the tight packing we had previously. I'd say that's bearable. For name events we are wasting two longs per event compared to the tightest packing I can imagine (i.e., 97+name vs 81+name). That's bad enough but I can live with that for now... We could actually improve packing of name events by declaring handle as: struct fanotify_fh { u8 type; u8 len; u8 fh[FANOTIFY_INLINE_FH_LEN]; }; This is a structure that has no padding requirements and so if we place two next to each other they will use just 36 bytes instead of 48. But then we have to play games with hiding pointer inside 'fh' like: char **fh_ext_ptr(struct fanotify_fh *fh) { return (char **)ALIGN((unsigned long)(fh->fh), __alignof__(char *)); } Probably it's worth it but I wouldn't bother for this series if you don't want to. Honza
On Thu 27-02-20 11:06:18, Amir Goldstein wrote: > > > So overall I think this would be better. The question is whether the > > > resulting code will really be more readable. I hope so because the > > > structures are definitely nicer this way and things belonging logically > > > together are now together. But you never know until you convert the code... > > > Would you be willing to try this refactoring? > > > > Yes, but I would like to know what you think about the two 6 byte holes > > Just let that space be wasted for the sake of nicer abstraction? > > It seems like too much to me. > > > > What if we unite the fh and name into one struct and keep a 32bit hash of > fh+name inside? > > This will allow us to mitigate the cost of memcmp of fh+name in merge > and get rid of objectid in fsnotify_event as you suggested. I definitely want to get rid of objectid in the long run but I wouldn't necessarily tie it to this series. What I had in mind to do for fanotify to speed up merging (in the light of your report) was to associate a hash with each fanotify event based on values we care about most (probably store it in the same word as fanotify event type) and compare based on this hash first. Possibly, we could also add a small hash table (say 128 entries) to each fanotify group based on this hash to speed up looking up candidates for merging. > struct fanotify_fh_name { > union { > struct { > u8 fh_type; > u8 fh_len; > u8 name_len; > u32 hash; > }; > u64 hash_len; > }; > union { > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > unsigned char *ext_fh; > }; > char name[0]; > }; So based on the above I wouldn't add just name hash to fanotify_fh_name at this point... Honza > struct fanotify_fid_event { > struct fanotify_event fae; > __kernel_fsid_t fsid; > struct fanotify_fh_name object_fh; /* name is empty */ > }; > > struct fanofify_name_event { > struct fanotify_fid_event ffe; > struct fanotify_fh_name dirent; > }; > > So the only anomaly is that we use struct fanotify_fh_name > to describe object_fh which never has a name. > > I think we can live with that and trying to beat that would be > over abstraction. > > Thoughts? > > Thanks, > Amir.
> > > struct fanotify_fh_name { > > union { > > struct { > > u8 fh_type; > > u8 fh_len; > > u8 name_len; > > u32 hash; > > }; > > u64 hash_len; > > }; > > union { > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > unsigned char *ext_fh; > > }; > > char name[0]; > > }; > > So based on the above I wouldn't add just name hash to fanotify_fh_name at > this point... > OK. but what do you think about tying name with fh as above? At least name_len gets to use the hole this way. I am trying this out now and it is really hard for me not to call the struct above fanotify_fid. IMO code looks much better when it is called this way. The problem is inconsistency with struct fanotify_event_info_fid which does include fsid, but I think we can live with that. Anyway, I'll prepare a version or two of the end result and let you see how it looks. Thanks, Amir.
On Thu 27-02-20 14:12:30, Amir Goldstein wrote: > > > > > struct fanotify_fh_name { > > > union { > > > struct { > > > u8 fh_type; > > > u8 fh_len; > > > u8 name_len; > > > u32 hash; > > > }; > > > u64 hash_len; > > > }; > > > union { > > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > > unsigned char *ext_fh; > > > }; > > > char name[0]; > > > }; > > > > So based on the above I wouldn't add just name hash to fanotify_fh_name at > > this point... > > > > OK. but what do you think about tying name with fh as above? > At least name_len gets to use the hole this way. Is saving that one byte for name_len really worth the packing? If anything, I'd rather do the fanotity_fh padding optimization I outlined in another email. That would save one long without any packing and the following u8 name_len would get packed tightly after the fanotify_fh by the compiler. Honza
On Thu, Feb 27, 2020 at 3:30 PM Jan Kara <jack@suse.cz> wrote: > > On Thu 27-02-20 14:12:30, Amir Goldstein wrote: > > > > > > > struct fanotify_fh_name { > > > > union { > > > > struct { > > > > u8 fh_type; > > > > u8 fh_len; > > > > u8 name_len; > > > > u32 hash; > > > > }; > > > > u64 hash_len; > > > > }; > > > > union { > > > > unsigned char fh[FANOTIFY_INLINE_FH_LEN]; > > > > unsigned char *ext_fh; > > > > }; > > > > char name[0]; > > > > }; > > > > > > So based on the above I wouldn't add just name hash to fanotify_fh_name at > > > this point... > > > > > > > OK. but what do you think about tying name with fh as above? > > At least name_len gets to use the hole this way. > > Is saving that one byte for name_len really worth the packing? If anything, > I'd rather do the fanotity_fh padding optimization I outlined in another > email. That would save one long without any packing and the following u8 > name_len would get packed tightly after the fanotify_fh by the compiler. > OK. I will try that and the non-inherited variant of perm/name event struct and see how it looks like. Thanks, Amir.
> > I'd rather do the fanotity_fh padding optimization I outlined in another > > email. That would save one long without any packing and the following u8 > > name_len would get packed tightly after the fanotify_fh by the compiler. > > > > OK. I will try that and the non-inherited variant of perm/name event struct > and see how it looks like. > Pushed sample code to branch fanotify_name-wip: b5e56d3e1358 fanotify: fanotify_perm_event inherits from fanotify_path_event 55041285b3b7 fanotify: divorce fanotify_path_event and fanotify_fid_event I opted for fanotify_name_event inherits from fanotify_fid_event, because it felt better this way. I wasn't sure about fanotify_perm_event inherits from fanotify_path_event, so did that is a separate patch so you can judge both variants. IMO, neither variant is that good or bad, so I could go with either. I do like the end result with your suggestions better than fanotify_name-v2. If you like this version, I will work the changes into the series. Thanks, Amir.
Hi Amir! On Sun 01-03-20 18:26:25, Amir Goldstein wrote: > > > I'd rather do the fanotity_fh padding optimization I outlined in another > > > email. That would save one long without any packing and the following u8 > > > name_len would get packed tightly after the fanotify_fh by the compiler. > > > > > > > OK. I will try that and the non-inherited variant of perm/name event struct > > and see how it looks like. > > > > Pushed sample code to branch fanotify_name-wip: > > b5e56d3e1358 fanotify: fanotify_perm_event inherits from fanotify_path_event > 55041285b3b7 fanotify: divorce fanotify_path_event and fanotify_fid_event Thanks for the work! > I opted for fanotify_name_event inherits from fanotify_fid_event, > because it felt better this way. I've commented on github in the patches - I'm not sure the inheritance really brings a significant benefit and it costs 6 bytes per name event. Maybe there can be more simplifications gained from the inheritance (but I think the move of fsid out of fanotify_fid mostly precludes that) but at this point it doesn't seem to be worth it to me. > I wasn't sure about fanotify_perm_event inherits from fanotify_path_event, > so did that is a separate patch so you can judge both variants. > IMO, neither variant is that good or bad, so I could go with either. Yeah, I don't think the inheritance is really worth the churn. > I do like the end result with your suggestions better than fanotify_name-v2. > If you like this version, I will work the changes into the series. Yes, overall the code look better! Thanks! Honza
On Thu, Mar 5, 2020 at 5:49 PM Jan Kara <jack@suse.cz> wrote: > > Hi Amir! > > On Sun 01-03-20 18:26:25, Amir Goldstein wrote: > > > > I'd rather do the fanotity_fh padding optimization I outlined in another > > > > email. That would save one long without any packing and the following u8 > > > > name_len would get packed tightly after the fanotify_fh by the compiler. > > > > > > > > > > OK. I will try that and the non-inherited variant of perm/name event struct > > > and see how it looks like. > > > > > > > Pushed sample code to branch fanotify_name-wip: > > > > b5e56d3e1358 fanotify: fanotify_perm_event inherits from fanotify_path_event > > 55041285b3b7 fanotify: divorce fanotify_path_event and fanotify_fid_event > > Thanks for the work! > > > I opted for fanotify_name_event inherits from fanotify_fid_event, > > because it felt better this way. > > I've commented on github in the patches - I'm not sure the inheritance > really brings a significant benefit and it costs 6 bytes per name event. > Maybe there can be more simplifications gained from the inheritance (but I > think the move of fsid out of fanotify_fid mostly precludes that) but at > this point it doesn't seem to be worth it to me. > As agreed on github discussion, the padding is a non issue. To see what the benefit of inherit fanotify_fid_event is, I did a test patch to get rid of it and pushed the result to fanotify_name-wip: * b7eb8314c61b - fanotify: do not inherit fanotify_name_event from fanotify_fid_event IMO, the removal of inheritance in this struct is artificial and brings no benefit. There is not a single line of code that improved IMO vs. several added helpers which abstract something that is pretty obvious. That said, I don't mind going with this variant. Let me you what your final call is. Thanks, Amir.
> To see what the benefit of inherit fanotify_fid_event is, I did a test patch > to get rid of it and pushed the result to fanotify_name-wip: > > * b7eb8314c61b - fanotify: do not inherit fanotify_name_event from > fanotify_fid_event > > IMO, the removal of inheritance in this struct is artificial and > brings no benefit. > There is not a single line of code that improved IMO vs. several added > helpers which abstract something that is pretty obvious. > > That said, I don't mind going with this variant. > Let me you what your final call is. > Eventually, it was easier to work the non-inherited variant into the series as the helpers aid with abstracting things as the series progresses and because object_fh is added to fanotify_name_event late in the series. So I went with your preference. Pushed the work to fanotify_name branch. Let me know if you want me to post v3. Thanks, Amir.
On Sun 08-03-20 09:29:08, Amir Goldstein wrote: > > To see what the benefit of inherit fanotify_fid_event is, I did a test patch > > to get rid of it and pushed the result to fanotify_name-wip: > > > > * b7eb8314c61b - fanotify: do not inherit fanotify_name_event from > > fanotify_fid_event > > > > IMO, the removal of inheritance in this struct is artificial and > > brings no benefit. > > There is not a single line of code that improved IMO vs. several added > > helpers which abstract something that is pretty obvious. > > > > That said, I don't mind going with this variant. > > Let me you what your final call is. > > > > Eventually, it was easier to work the non-inherited variant into the series > as the helpers aid with abstracting things as the series progresses and > because object_fh is added to fanotify_name_event late in the series. > So I went with your preference. > > Pushed the work to fanotify_name branch. > Let me know if you want me to post v3. So I went through the patches - had only minor comments for most of them. Can you post the next revision by email and I'll pickup at least the obvious preparatory patches to my tree. Thanks! Honza
> > Pushed the work to fanotify_name branch. > > Let me know if you want me to post v3. > > So I went through the patches - had only minor comments for most of them. > Can you post the next revision by email and I'll pickup at least the > obvious preparatory patches to my tree. Thanks! > Will do. Most of your comments were minor, but the last comments on FAN_REPORT_NAME patch send me to do some homework. I wonder if you would like me to post only the FAN_DIR_MODIFY patches, which seem ready for prime time and defer the FAN_REPORT_NAME changes to the next merge window? Or do you prefer to merge all the planned event name info API changes at once? Thanks, Amir.
On Wed 18-03-20 20:50:39, Amir Goldstein wrote: > > > Pushed the work to fanotify_name branch. > > > Let me know if you want me to post v3. > > > > So I went through the patches - had only minor comments for most of them. > > Can you post the next revision by email and I'll pickup at least the > > obvious preparatory patches to my tree. Thanks! > > > > Will do. > Most of your comments were minor, but the last comments on > FAN_REPORT_NAME patch send me to do some homework. > > I wonder if you would like me to post only the FAN_DIR_MODIFY > patches, which seem ready for prime time and defer the > FAN_REPORT_NAME changes to the next merge window? Yes, that's certainly one option. AFAIU the patches, the FAN_DIR_MODIFY is completely independent of the new feature of groups with FAN_REPORT_NAME, isn't it? Honza
On Thu, Mar 19, 2020 at 11:30 AM Jan Kara <jack@suse.cz> wrote: > > On Wed 18-03-20 20:50:39, Amir Goldstein wrote: > > > > Pushed the work to fanotify_name branch. > > > > Let me know if you want me to post v3. > > > > > > So I went through the patches - had only minor comments for most of them. > > > Can you post the next revision by email and I'll pickup at least the > > > obvious preparatory patches to my tree. Thanks! > > > > > > > Will do. > > Most of your comments were minor, but the last comments on > > FAN_REPORT_NAME patch send me to do some homework. > > > > I wonder if you would like me to post only the FAN_DIR_MODIFY > > patches, which seem ready for prime time and defer the > > FAN_REPORT_NAME changes to the next merge window? > > Yes, that's certainly one option. AFAIU the patches, the FAN_DIR_MODIFY is > completely independent of the new feature of groups with FAN_REPORT_NAME, > isn't it? > That is correct. From UAPI perspective, I wanted to tell the final story and show that it looks coherent, but even the man-page draft changes are in two separate patches: https://github.com/amir73il/man-pages/commits/fanotify_name So there is no problem with merging them seperately. Will post only FAN_DIR_MODIFY for v3. Thanks, Amir.
On Wed, Mar 18, 2020 at 8:50 PM Amir Goldstein <amir73il@gmail.com> wrote: > > > > Pushed the work to fanotify_name branch. > > > Let me know if you want me to post v3. > > > > So I went through the patches - had only minor comments for most of them. > > Can you post the next revision by email and I'll pickup at least the > > obvious preparatory patches to my tree. Thanks! > > > > Will do. > Most of your comments were minor, but the last comments on > FAN_REPORT_NAME patch send me to do some homework. > I know this patch is for next next release, but I was just investigating so wanted to publish the results. For the records, your question about the FAN_REPORT_NAME patch was: "... this seems to be somewhat duplicating the functionality of __fsnotify_parent(). Can't we somehow join these paths?" I remembered that I started with this approach and moved to taking name snapshots inside fanotify event handler for a reason, but did not remember what it was. So I went digging back and found that I wanted to avoid the situation where in mount/sb marks events are reported in two flavors, one with name and one without name. I ended up with something that works, but the logic is quite hard to follow and to document. So decided it is best to go back to fsnotify_parent() approach and let the two flavors of events be reported for sb/mount marks. I pushed the end result to branch fanotify_name and adjusted the LTP test to expect the extra events. I will see how that ends up looking in the man page. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 1f60823931b7..3bc28f08aad1 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -27,7 +27,7 @@ static bool should_merge(struct fsnotify_event *old_fsn, new = FANOTIFY_E(new_fsn); if (old_fsn->tag != new_fsn->tag || old->pid != new->pid || - old->fh_type != new->fh_type || old->fh_len != new->fh_len) + old->fh.type != new->fh.type || old->fh.len != new->fh.len) return false; if (fanotify_event_has_path(old)) { @@ -43,7 +43,8 @@ static bool should_merge(struct fsnotify_event *old_fsn, * unlink pair or rmdir+create pair of events. */ return (old->mask & FS_ISDIR) == (new->mask & FS_ISDIR) && - fanotify_fid_equal(&old->fid, &new->fid, old->fh_len); + fanotify_fsid_equal(&old->fsid, &new->fsid) && + fanotify_fid_equal(&old->fid, &new->fid, old->fh.len); } /* Do not merge events if we failed to encode fid */ @@ -213,18 +214,18 @@ static u32 fanotify_group_event_mask(struct fsnotify_group *group, return test_mask & user_mask; } -static int fanotify_encode_fid(struct fanotify_event *event, - struct inode *inode, gfp_t gfp, - __kernel_fsid_t *fsid) +static struct fanotify_fid_hdr fanotify_encode_fid(struct fanotify_fid *fid, + struct inode *inode, + gfp_t gfp) { - struct fanotify_fid *fid = &event->fid; + struct fanotify_fid_hdr fh = { }; int dwords, bytes = 0; - int err, type; + int err; fid->ext_fh = NULL; dwords = 0; err = -ENOENT; - type = exportfs_encode_inode_fh(inode, NULL, &dwords, NULL); + fh.type = exportfs_encode_inode_fh(inode, NULL, &dwords, NULL); if (!dwords) goto out_err; @@ -237,26 +238,25 @@ static int fanotify_encode_fid(struct fanotify_event *event, goto out_err; } - type = exportfs_encode_inode_fh(inode, fanotify_fid_fh(fid, bytes), - &dwords, NULL); + fh.type = exportfs_encode_inode_fh(inode, fanotify_fid_fh(fid, bytes), + &dwords, NULL); err = -EINVAL; - if (!type || type == FILEID_INVALID || bytes != dwords << 2) + if (!fh.type || fh.type == FILEID_INVALID || bytes != dwords << 2) goto out_err; - fid->fsid = *fsid; - event->fh_len = bytes; + fh.len = bytes; - return type; + return fh; out_err: - pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, " - "type=%d, bytes=%d, err=%i)\n", - fsid->val[0], fsid->val[1], type, bytes, err); + pr_warn_ratelimited("fanotify: failed to encode fid (type=%d, len=%d, err=%i)\n", + fh.type, bytes, err); kfree(fid->ext_fh); fid->ext_fh = NULL; - event->fh_len = 0; + fh.type = FILEID_INVALID; + fh.len = 0; - return FILEID_INVALID; + return fh; } /* @@ -327,16 +327,18 @@ init: __maybe_unused event->pid = get_pid(task_pid(current)); else event->pid = get_pid(task_tgid(current)); - event->fh_len = 0; + event->fh.len = 0; + if (fsid) + event->fsid = *fsid; if (id && FAN_GROUP_FLAG(group, FAN_REPORT_FID)) { /* Report the event without a file identifier on encode error */ event->fh_type = fanotify_encode_fid(event, id, gfp, fsid); } else if (path) { - event->fh_type = FILEID_ROOT; + event->fh.type = FILEID_ROOT; event->path = *path; path_get(path); } else { - event->fh_type = FILEID_INVALID; + event->fh.type = FILEID_INVALID; event->path.mnt = NULL; event->path.dentry = NULL; } @@ -485,7 +487,7 @@ static void fanotify_free_event(struct fsnotify_event *fsn_event) event = FANOTIFY_E(fsn_event); if (fanotify_event_has_path(event)) path_put(&event->path); - else if (fanotify_event_has_ext_fh(event)) + else if (fanotify_fid_has_ext_fh(&event->fh)) kfree(event->fid.ext_fh); put_pid(event->pid); if (fanotify_is_perm_event(event->mask)) { diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 68b30504284c..4fee002235b6 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -18,10 +18,10 @@ enum { /* * 3 dwords are sufficient for most local fs (64bit ino, 32bit generation). - * For 32bit arch, fid increases the size of fanotify_event by 12 bytes and - * fh_* fields increase the size of fanotify_event by another 4 bytes. - * For 64bit arch, fid increases the size of fanotify_fid by 8 bytes and - * fh_* fields are packed in a hole after mask. + * For 32bit arch, fsid and fid increase the size of fanotify_event by 12 bytes + * and fh.* fields increase the size of fanotify_event by another 4 bytes. + * For 64bit arch, fanotify_fid is the same size as struct path, fsid increases + * fanotify_event by 8 bytes and fh.* fields are packed in a hole after mask. */ #if BITS_PER_LONG == 32 #define FANOTIFY_INLINE_FH_LEN (3 << 2) @@ -29,28 +29,46 @@ enum { #define FANOTIFY_INLINE_FH_LEN (4 << 2) #endif +struct fanotify_fid_hdr { + u8 type; + u8 len; +}; + struct fanotify_fid { - __kernel_fsid_t fsid; union { unsigned char fh[FANOTIFY_INLINE_FH_LEN]; unsigned char *ext_fh; }; }; +static inline bool fanotify_fid_has_fh(struct fanotify_fid_hdr *fh) +{ + return fh->type != FILEID_ROOT && fh->type != FILEID_INVALID; +} + +static inline bool fanotify_fid_has_ext_fh(struct fanotify_fid_hdr *fh) +{ + return fanotify_fid_has_fh(fh) && fh->len > FANOTIFY_INLINE_FH_LEN; +} + static inline void *fanotify_fid_fh(struct fanotify_fid *fid, unsigned int fh_len) { return fh_len <= FANOTIFY_INLINE_FH_LEN ? fid->fh : fid->ext_fh; } +static inline bool fanotify_fsid_equal(__kernel_fsid_t *fsid1, + __kernel_fsid_t *fsid2) +{ + return fsid1->val[0] == fsid1->val[0] && fsid2->val[1] == fsid2->val[1]; +} + static inline bool fanotify_fid_equal(struct fanotify_fid *fid1, struct fanotify_fid *fid2, unsigned int fh_len) { - return fid1->fsid.val[0] == fid2->fsid.val[0] && - fid1->fsid.val[1] == fid2->fsid.val[1] && - !memcmp(fanotify_fid_fh(fid1, fh_len), - fanotify_fid_fh(fid2, fh_len), fh_len); + return !memcmp(fanotify_fid_fh(fid1, fh_len), + fanotify_fid_fh(fid2, fh_len), fh_len); } /* @@ -63,13 +81,13 @@ struct fanotify_event { u32 mask; /* * Those fields are outside fanotify_fid to pack fanotify_event nicely - * on 64bit arch and to use fh_type as an indication of whether path + * on 64bit arch and to use fh.type as an indication of whether path * or fid are used in the union: * FILEID_ROOT (0) for path, > 0 for fid, FILEID_INVALID for neither. */ - u8 fh_type; - u8 fh_len; + struct fanotify_fid_hdr fh; u16 pad; + __kernel_fsid_t fsid; union { /* * We hold ref to this path so it may be dereferenced at any @@ -88,24 +106,12 @@ struct fanotify_event { static inline bool fanotify_event_has_path(struct fanotify_event *event) { - return event->fh_type == FILEID_ROOT; + return event->fh.type == FILEID_ROOT; } static inline bool fanotify_event_has_fid(struct fanotify_event *event) { - return event->fh_type != FILEID_ROOT && - event->fh_type != FILEID_INVALID; -} - -static inline bool fanotify_event_has_ext_fh(struct fanotify_event *event) -{ - return fanotify_event_has_fid(event) && - event->fh_len > FANOTIFY_INLINE_FH_LEN; -} - -static inline void *fanotify_event_fh(struct fanotify_event *event) -{ - return fanotify_fid_fh(&event->fid, event->fh_len); + return fanotify_fid_has_fh(&event->fh); } /* diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 0aa362b88550..beb9f0661a7c 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -51,14 +51,19 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly; #define FANOTIFY_EVENT_ALIGN 4 +static int fanotify_fid_info_len(struct fanotify_fid_hdr *fh) +{ + return roundup(sizeof(struct fanotify_event_info_fid) + + sizeof(struct file_handle) + fh->len, + FANOTIFY_EVENT_ALIGN); +} + static int fanotify_event_info_len(struct fanotify_event *event) { if (!fanotify_event_has_fid(event)) return 0; - return roundup(sizeof(struct fanotify_event_info_fid) + - sizeof(struct file_handle) + event->fh_len, - FANOTIFY_EVENT_ALIGN); + return fanotify_fid_info_len(&event->fh); } /* @@ -204,13 +209,14 @@ static int process_access_response(struct fsnotify_group *group, return -ENOENT; } -static int copy_fid_to_user(struct fanotify_event *event, char __user *buf) +static int copy_fid_to_user(__kernel_fsid_t *fsid, struct fanotify_fid_hdr *fh, + struct fanotify_fid *fid, char __user *buf) { struct fanotify_event_info_fid info = { }; struct file_handle handle = { }; - unsigned char bounce[FANOTIFY_INLINE_FH_LEN], *fh; - size_t fh_len = event->fh_len; - size_t len = fanotify_event_info_len(event); + unsigned char bounce[FANOTIFY_INLINE_FH_LEN], *data; + size_t fh_len = fh->len; + size_t len = fanotify_fid_info_len(fh); if (!len) return 0; @@ -221,13 +227,13 @@ static int copy_fid_to_user(struct fanotify_event *event, char __user *buf) /* Copy event info fid header followed by vaiable sized file handle */ info.hdr.info_type = FAN_EVENT_INFO_TYPE_FID; info.hdr.len = len; - info.fsid = event->fid.fsid; + info.fsid = *fsid; if (copy_to_user(buf, &info, sizeof(info))) return -EFAULT; buf += sizeof(info); len -= sizeof(info); - handle.handle_type = event->fh_type; + handle.handle_type = fh->type; handle.handle_bytes = fh_len; if (copy_to_user(buf, &handle, sizeof(handle))) return -EFAULT; @@ -238,12 +244,12 @@ static int copy_fid_to_user(struct fanotify_event *event, char __user *buf) * For an inline fh, copy through stack to exclude the copy from * usercopy hardening protections. */ - fh = fanotify_event_fh(event); + data = fanotify_fid_fh(fid, fh_len); if (fh_len <= FANOTIFY_INLINE_FH_LEN) { - memcpy(bounce, fh, fh_len); - fh = bounce; + memcpy(bounce, data, fh_len); + data = bounce; } - if (copy_to_user(buf, fh, fh_len)) + if (copy_to_user(buf, data, fh_len)) return -EFAULT; /* Pad with 0's */ @@ -301,7 +307,8 @@ static ssize_t copy_event_to_user(struct fsnotify_group *group, if (fanotify_event_has_path(event)) { fd_install(fd, f); } else if (fanotify_event_has_fid(event)) { - ret = copy_fid_to_user(event, buf + FAN_EVENT_METADATA_LEN); + ret = copy_fid_to_user(&event->fsid, &event->fh, &event->fid, + buf + FAN_EVENT_METADATA_LEN); if (ret < 0) return ret; }
For some events, we are going to encode both child and parent fid's, so we need to do a little refactoring of struct fanotify_event and fid helper functions. Move fsid member from struct fanotify_fid out to struct fanotify_event, so we can store fsid once for two encoded fid's (we will only encode parent if it is on the same filesystem). This does not change the size of struct fanotify_event because struct fanotify_fid is still bigger than struct path on 32bit arch and is the same size as struct path (16 bytes) on 64bit arch. Group fh_len and fh_type as struct fanotify_fid_hdr. Pass struct fanotify_fid and struct fanotify_fid_hdr to helpers fanotify_encode_fid() and copy_fid_to_user() instead of passing the containing fanotify_event struct. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.c | 48 +++++++++++++------------ fs/notify/fanotify/fanotify.h | 58 ++++++++++++++++-------------- fs/notify/fanotify/fanotify_user.c | 35 ++++++++++-------- 3 files changed, 78 insertions(+), 63 deletions(-)