On Wed, Feb 26, 2020 at 12:23 PM Jan Kara <jack@xxxxxxx> 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@xxxxxxxxx>
>
> ...
>
> > @@ -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.
