| Message ID | 20210804160612.3575505-20-krisman@collabora.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | File system wide monitoring | expand |
On Wed 04-08-21 12:06:08, Gabriel Krisman Bertazi wrote: > Plumb the pieces to add a FID report to error records. Since all error > event memory must be pre-allocated, we estimate a file handler size and > if it is insuficient, we report an invalid FID and increase the > prediction for the next error slot allocation. Hum, cannot we just preallocate MAX_HANDLE_SZ? It is wasteful but then I don't expect error marks would be that frequent for this to really matter and the code would be simpler. Honza > > For errors that don't expose a file handler report it with an invalid > FID. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> > --- > fs/notify/fanotify/fanotify.c | 27 +++++++++++++++++++++++++++ > fs/notify/fanotify/fanotify.h | 12 ++++++++++++ > fs/notify/fanotify/fanotify_user.c | 19 ++++++++++++++++++- > 3 files changed, 57 insertions(+), 1 deletion(-) > > diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c > index 4e9e271a4394..2fd30b5eb9d3 100644 > --- a/fs/notify/fanotify/fanotify.c > +++ b/fs/notify/fanotify/fanotify.c > @@ -705,7 +705,9 @@ static void fanotify_insert_error_event(struct fsnotify_group *group, > { > const struct fs_error_report *report = (struct fs_error_report *) data; > struct fanotify_event *fae = FANOTIFY_E(event); > + struct inode *inode = report->inode; > struct fanotify_error_event *fee; > + int fh_len; > > /* This might be an unexpected type of event (i.e. overflow). */ > if (!fanotify_is_error_event(fae->mask)) > @@ -715,6 +717,31 @@ static void fanotify_insert_error_event(struct fsnotify_group *group, > fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; > fee->error = report->error; > fee->err_count = 1; > + fee->fsid = fee->sb_mark->fsn_mark.connector->fsid; > + > + /* > + * Error reporting needs to happen in atomic context. If this > + * inode's file handler length is more than we initially > + * predicted, there is nothing better we can do than report the > + * error with a bad file handler. > + */ > + fh_len = fanotify_encode_fh_len(inode); > + if (fh_len > fee->sb_mark->pred_fh_len) { > + pr_warn_ratelimited( > + "FH overflows error event. Drop inode information.\n"); > + /* > + * Update the handler size prediction for the next error > + * event allocation. This reduces the chance of another > + * overflow. > + */ > + fee->sb_mark->pred_fh_len = fh_len; > + > + /* For the current error, ignore the inode information. */ > + inode = NULL; > + fh_len = fanotify_encode_fh_len(NULL); > + } > + > + fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0); > } > > /* > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index 8929ea50f96f..e4e7968b70ee 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -142,6 +142,7 @@ FANOTIFY_MARK_FLAG(SB_MARK); > > struct fanotify_sb_mark { > struct fsnotify_mark fsn_mark; > + size_t pred_fh_len; > struct fanotify_error_event *fee_slot; > }; > > @@ -227,6 +228,13 @@ struct fanotify_error_event { > u32 err_count; /* Suppressed errors count */ > > struct fanotify_sb_mark *sb_mark; /* Back reference to the mark. */ > + > + __kernel_fsid_t fsid; /* FSID this error refers to. */ > + /* > + * object_fh is followed by a variable sized buffer, so it must > + * be the last element of this structure. > + */ > + struct fanotify_fh object_fh; > }; > > static inline struct fanotify_error_event * > @@ -241,6 +249,8 @@ static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event) > return &FANOTIFY_FE(event)->fsid; > else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME) > return &FANOTIFY_NE(event)->fsid; > + else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR) > + return &FANOTIFY_EE(event)->fsid; > else > return NULL; > } > @@ -252,6 +262,8 @@ static inline struct fanotify_fh *fanotify_event_object_fh( > return &FANOTIFY_FE(event)->object_fh; > else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME) > return fanotify_info_file_fh(&FANOTIFY_NE(event)->info); > + else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR) > + return &FANOTIFY_EE(event)->object_fh; > else > return NULL; > } > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index e7fe6bc61b6f..74def82630bb 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -172,8 +172,25 @@ static struct fanotify_error_event *fanotify_alloc_error_event( > > { > struct fanotify_error_event *fee; > + struct super_block *sb; > > - fee = kzalloc(sizeof(*fee), GFP_KERNEL_ACCOUNT); > + if (!sb_mark->pred_fh_len) { > + /* > + * The handler size is initially predicted to be the > + * same size as the root inode file handler. It can be > + * increased later if a larger file handler is found. > + */ > + sb = container_of(sb_mark->fsn_mark.connector->obj, > + struct super_block, s_fsnotify_marks); > + sb_mark->pred_fh_len = > + fanotify_encode_fh_len(sb->s_root->d_inode); > + } > + > + /* Guarantee there is always space to report an invalid FID. */ > + if (sb_mark->pred_fh_len < FANOTIFY_NULL_FH_LEN) > + sb_mark->pred_fh_len = FANOTIFY_NULL_FH_LEN; > + > + fee = kzalloc(sizeof(*fee) + sb_mark->pred_fh_len, GFP_KERNEL_ACCOUNT); > if (!fee) > return NULL; > > -- > 2.32.0 >
On Wed 04-08-21 12:06:08, Gabriel Krisman Bertazi wrote: > Plumb the pieces to add a FID report to error records. Since all error > event memory must be pre-allocated, we estimate a file handler size and > if it is insuficient, we report an invalid FID and increase the > prediction for the next error slot allocation. > > For errors that don't expose a file handler report it with an invalid > FID. > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> ... > @@ -715,6 +717,31 @@ static void fanotify_insert_error_event(struct fsnotify_group *group, > fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; > fee->error = report->error; > fee->err_count = 1; > + fee->fsid = fee->sb_mark->fsn_mark.connector->fsid; > + > + /* > + * Error reporting needs to happen in atomic context. If this > + * inode's file handler length is more than we initially > + * predicted, there is nothing better we can do than report the > + * error with a bad file handler. > + */ > + fh_len = fanotify_encode_fh_len(inode); > + if (fh_len > fee->sb_mark->pred_fh_len) { > + pr_warn_ratelimited( > + "FH overflows error event. Drop inode information.\n"); > + /* > + * Update the handler size prediction for the next error > + * event allocation. This reduces the chance of another > + * overflow. > + */ > + fee->sb_mark->pred_fh_len = fh_len; > + > + /* For the current error, ignore the inode information. */ > + inode = NULL; > + fh_len = fanotify_encode_fh_len(NULL); > + } > + > + fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0); > } I don't think calling fanotify_encode_fh() from under notification_lock is a good practice. It calls into a filesystem and god knows which locks it may need to take to get necessary info... See my reply to patch 18 how we could handle that better. Honza
diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c index 4e9e271a4394..2fd30b5eb9d3 100644 --- a/fs/notify/fanotify/fanotify.c +++ b/fs/notify/fanotify/fanotify.c @@ -705,7 +705,9 @@ static void fanotify_insert_error_event(struct fsnotify_group *group, { const struct fs_error_report *report = (struct fs_error_report *) data; struct fanotify_event *fae = FANOTIFY_E(event); + struct inode *inode = report->inode; struct fanotify_error_event *fee; + int fh_len; /* This might be an unexpected type of event (i.e. overflow). */ if (!fanotify_is_error_event(fae->mask)) @@ -715,6 +717,31 @@ static void fanotify_insert_error_event(struct fsnotify_group *group, fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR; fee->error = report->error; fee->err_count = 1; + fee->fsid = fee->sb_mark->fsn_mark.connector->fsid; + + /* + * Error reporting needs to happen in atomic context. If this + * inode's file handler length is more than we initially + * predicted, there is nothing better we can do than report the + * error with a bad file handler. + */ + fh_len = fanotify_encode_fh_len(inode); + if (fh_len > fee->sb_mark->pred_fh_len) { + pr_warn_ratelimited( + "FH overflows error event. Drop inode information.\n"); + /* + * Update the handler size prediction for the next error + * event allocation. This reduces the chance of another + * overflow. + */ + fee->sb_mark->pred_fh_len = fh_len; + + /* For the current error, ignore the inode information. */ + inode = NULL; + fh_len = fanotify_encode_fh_len(NULL); + } + + fanotify_encode_fh(&fee->object_fh, inode, fh_len, NULL, 0); } /* diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index 8929ea50f96f..e4e7968b70ee 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -142,6 +142,7 @@ FANOTIFY_MARK_FLAG(SB_MARK); struct fanotify_sb_mark { struct fsnotify_mark fsn_mark; + size_t pred_fh_len; struct fanotify_error_event *fee_slot; }; @@ -227,6 +228,13 @@ struct fanotify_error_event { u32 err_count; /* Suppressed errors count */ struct fanotify_sb_mark *sb_mark; /* Back reference to the mark. */ + + __kernel_fsid_t fsid; /* FSID this error refers to. */ + /* + * object_fh is followed by a variable sized buffer, so it must + * be the last element of this structure. + */ + struct fanotify_fh object_fh; }; static inline struct fanotify_error_event * @@ -241,6 +249,8 @@ static inline __kernel_fsid_t *fanotify_event_fsid(struct fanotify_event *event) return &FANOTIFY_FE(event)->fsid; else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME) return &FANOTIFY_NE(event)->fsid; + else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR) + return &FANOTIFY_EE(event)->fsid; else return NULL; } @@ -252,6 +262,8 @@ static inline struct fanotify_fh *fanotify_event_object_fh( return &FANOTIFY_FE(event)->object_fh; else if (event->type == FANOTIFY_EVENT_TYPE_FID_NAME) return fanotify_info_file_fh(&FANOTIFY_NE(event)->info); + else if (event->type == FANOTIFY_EVENT_TYPE_FS_ERROR) + return &FANOTIFY_EE(event)->object_fh; else return NULL; } diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index e7fe6bc61b6f..74def82630bb 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -172,8 +172,25 @@ static struct fanotify_error_event *fanotify_alloc_error_event( { struct fanotify_error_event *fee; + struct super_block *sb; - fee = kzalloc(sizeof(*fee), GFP_KERNEL_ACCOUNT); + if (!sb_mark->pred_fh_len) { + /* + * The handler size is initially predicted to be the + * same size as the root inode file handler. It can be + * increased later if a larger file handler is found. + */ + sb = container_of(sb_mark->fsn_mark.connector->obj, + struct super_block, s_fsnotify_marks); + sb_mark->pred_fh_len = + fanotify_encode_fh_len(sb->s_root->d_inode); + } + + /* Guarantee there is always space to report an invalid FID. */ + if (sb_mark->pred_fh_len < FANOTIFY_NULL_FH_LEN) + sb_mark->pred_fh_len = FANOTIFY_NULL_FH_LEN; + + fee = kzalloc(sizeof(*fee) + sb_mark->pred_fh_len, GFP_KERNEL_ACCOUNT); if (!fee) return NULL;
Plumb the pieces to add a FID report to error records. Since all error event memory must be pre-allocated, we estimate a file handler size and if it is insuficient, we report an invalid FID and increase the prediction for the next error slot allocation. For errors that don't expose a file handler report it with an invalid FID. Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com> --- fs/notify/fanotify/fanotify.c | 27 +++++++++++++++++++++++++++ fs/notify/fanotify/fanotify.h | 12 ++++++++++++ fs/notify/fanotify/fanotify_user.c | 19 ++++++++++++++++++- 3 files changed, 57 insertions(+), 1 deletion(-)