All of lore.kernel.org
 help / color / mirror / Atom feed
* [bug report] fanotify: fix copy_event_to_user() fid error clean up
@ 2021-07-21 12:54 Dan Carpenter
  2021-07-21 22:38 ` Matthew Bobrowski
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-07-21 12:54 UTC (permalink / raw)
  To: repnop; +Cc: Amir Goldstein, linux-fsdevel

Hello Matthew Bobrowski,

The patch f644bc449b37: "fanotify: fix copy_event_to_user() fid error
clean up" from Jun 11, 2021, leads to the following static checker
warning:

	fs/notify/fanotify/fanotify_user.c:533 copy_event_to_user()
	error: we previously assumed 'f' could be null (see line 462)

fs/notify/fanotify/fanotify_user.c
    401 static ssize_t copy_event_to_user(struct fsnotify_group *group,
    402 				  struct fanotify_event *event,
    403 				  char __user *buf, size_t count)
    404 {
    405 	struct fanotify_event_metadata metadata;
    406 	struct path *path = fanotify_event_path(event);
    407 	struct fanotify_info *info = fanotify_event_info(event);
    408 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
    409 	struct file *f = NULL;
    410 	int ret, fd = FAN_NOFD;
    411 	int info_type = 0;
    412 
    413 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
    414 
    415 	metadata.event_len = FAN_EVENT_METADATA_LEN +
    416 				fanotify_event_info_len(fid_mode, event);
    417 	metadata.metadata_len = FAN_EVENT_METADATA_LEN;
    418 	metadata.vers = FANOTIFY_METADATA_VERSION;
    419 	metadata.reserved = 0;
    420 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
    421 	metadata.pid = pid_vnr(event->pid);
    422 	/*
    423 	 * For an unprivileged listener, event->pid can be used to identify the
    424 	 * events generated by the listener process itself, without disclosing
    425 	 * the pids of other processes.
    426 	 */
    427 	if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
    428 	    task_tgid(current) != event->pid)
    429 		metadata.pid = 0;
    430 
    431 	/*
    432 	 * For now, fid mode is required for an unprivileged listener and
    433 	 * fid mode does not report fd in events.  Keep this check anyway
    434 	 * for safety in case fid mode requirement is relaxed in the future
    435 	 * to allow unprivileged listener to get events with no fd and no fid.
    436 	 */
    437 	if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
    438 	    path && path->mnt && path->dentry) {
    439 		fd = create_fd(group, path, &f);
    440 		if (fd < 0)
    441 			return fd;
    442 	}

"f" is NULL on the else path

    443 	metadata.fd = fd;
    444 
    445 	ret = -EFAULT;
    446 	/*
    447 	 * Sanity check copy size in case get_one_event() and
    448 	 * event_len sizes ever get out of sync.
    449 	 */
    450 	if (WARN_ON_ONCE(metadata.event_len > count))
    451 		goto out_close_fd;
    452 
    453 	if (copy_to_user(buf, &metadata, FAN_EVENT_METADATA_LEN))
    454 		goto out_close_fd;
                        ^^^^^^^^^^^^^^^^^
This is problematic

    455 
    456 	buf += FAN_EVENT_METADATA_LEN;
    457 	count -= FAN_EVENT_METADATA_LEN;
    458 
    459 	if (fanotify_is_perm_event(event->mask))
    460 		FANOTIFY_PERM(event)->fd = fd;
    461 
    462 	if (f)
                   ^^^

    463 		fd_install(fd, f);
    464 
    465 	/* Event info records order is: dir fid + name, child fid */
    466 	if (fanotify_event_dir_fh_len(event)) {
    467 		info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
    468 					     FAN_EVENT_INFO_TYPE_DFID;
    469 		ret = copy_info_to_user(fanotify_event_fsid(event),
    470 					fanotify_info_dir_fh(info),
    471 					info_type, fanotify_info_name(info),
    472 					info->name_len, buf, count);
    473 		if (ret < 0)
    474 			goto out_close_fd;
    475 
    476 		buf += ret;
    477 		count -= ret;
    478 	}
    479 
    480 	if (fanotify_event_object_fh_len(event)) {
    481 		const char *dot = NULL;
    482 		int dot_len = 0;
    483 
    484 		if (fid_mode == FAN_REPORT_FID || info_type) {
    485 			/*
    486 			 * With only group flag FAN_REPORT_FID only type FID is
    487 			 * reported. Second info record type is always FID.
    488 			 */
    489 			info_type = FAN_EVENT_INFO_TYPE_FID;
    490 		} else if ((fid_mode & FAN_REPORT_NAME) &&
    491 			   (event->mask & FAN_ONDIR)) {
    492 			/*
    493 			 * With group flag FAN_REPORT_NAME, if name was not
    494 			 * recorded in an event on a directory, report the
    495 			 * name "." with info type DFID_NAME.
    496 			 */
    497 			info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
    498 			dot = ".";
    499 			dot_len = 1;
    500 		} else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
    501 			   (event->mask & FAN_ONDIR)) {
    502 			/*
    503 			 * With group flag FAN_REPORT_DIR_FID, a single info
    504 			 * record has type DFID for directory entry modification
    505 			 * event and for event on a directory.
    506 			 */
    507 			info_type = FAN_EVENT_INFO_TYPE_DFID;
    508 		} else {
    509 			/*
    510 			 * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
    511 			 * a single info record has type FID for event on a
    512 			 * non-directory, when there is no directory to report.
    513 			 * For example, on FAN_DELETE_SELF event.
    514 			 */
    515 			info_type = FAN_EVENT_INFO_TYPE_FID;
    516 		}
    517 
    518 		ret = copy_info_to_user(fanotify_event_fsid(event),
    519 					fanotify_event_object_fh(event),
    520 					info_type, dot, dot_len, buf, count);
    521 		if (ret < 0)
    522 			goto out_close_fd;
                                ^^^^^^^^^^^^^^^^^


    523 
    524 		buf += ret;
    525 		count -= ret;
    526 	}
    527 
    528 	return metadata.event_len;
    529 
    530 out_close_fd:
    531 	if (fd != FAN_NOFD) {
    532 		put_unused_fd(fd);
--> 533 		fput(f);
                        ^^^^^^^
This leads to a NULL dereference

    534 	}
    535 	return ret;
    536 }

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] fanotify: fix copy_event_to_user() fid error clean up
  2021-07-21 12:54 [bug report] fanotify: fix copy_event_to_user() fid error clean up Dan Carpenter
@ 2021-07-21 22:38 ` Matthew Bobrowski
  2021-07-22  9:01   ` Amir Goldstein
  0 siblings, 1 reply; 6+ messages in thread
From: Matthew Bobrowski @ 2021-07-21 22:38 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Amir Goldstein, linux-fsdevel

On Wed, Jul 21, 2021 at 05:54:07AM -0700, Dan Carpenter wrote:
> Hello Matthew Bobrowski,
> 
> The patch f644bc449b37: "fanotify: fix copy_event_to_user() fid error
> clean up" from Jun 11, 2021, leads to the following static checker
> warning:
> 
> 	fs/notify/fanotify/fanotify_user.c:533 copy_event_to_user()
> 	error: we previously assumed 'f' could be null (see line 462)

I've made a couple comments below. What am I missing?

> fs/notify/fanotify/fanotify_user.c
>     401 static ssize_t copy_event_to_user(struct fsnotify_group *group,
>     402 				  struct fanotify_event *event,
>     403 				  char __user *buf, size_t count)
>     404 {
>     405 	struct fanotify_event_metadata metadata;
>     406 	struct path *path = fanotify_event_path(event);
>     407 	struct fanotify_info *info = fanotify_event_info(event);
>     408 	unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
>     409 	struct file *f = NULL;
>     410 	int ret, fd = FAN_NOFD;
>     411 	int info_type = 0;
>     412 
>     413 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>     414 
>     415 	metadata.event_len = FAN_EVENT_METADATA_LEN +
>     416 				fanotify_event_info_len(fid_mode, event);
>     417 	metadata.metadata_len = FAN_EVENT_METADATA_LEN;
>     418 	metadata.vers = FANOTIFY_METADATA_VERSION;
>     419 	metadata.reserved = 0;
>     420 	metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
>     421 	metadata.pid = pid_vnr(event->pid);
>     422 	/*
>     423 	 * For an unprivileged listener, event->pid can be used to identify the
>     424 	 * events generated by the listener process itself, without disclosing
>     425 	 * the pids of other processes.
>     426 	 */
>     427 	if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
>     428 	    task_tgid(current) != event->pid)
>     429 		metadata.pid = 0;
>     430 
>     431 	/*
>     432 	 * For now, fid mode is required for an unprivileged listener and
>     433 	 * fid mode does not report fd in events.  Keep this check anyway
>     434 	 * for safety in case fid mode requirement is relaxed in the future
>     435 	 * to allow unprivileged listener to get events with no fd and no fid.
>     436 	 */
>     437 	if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
>     438 	    path && path->mnt && path->dentry) {
>     439 		fd = create_fd(group, path, &f);
>     440 		if (fd < 0)
>     441 			return fd;
>     442 	}
> 
> "f" is NULL on the else path

Uh ha, although "fd" on the else path also remains set to the initial value
of FAN_NOFD, right?

>     443 	metadata.fd = fd;
>     444 
>     445 	ret = -EFAULT;
>     446 	/*
>     447 	 * Sanity check copy size in case get_one_event() and
>     448 	 * event_len sizes ever get out of sync.
>     449 	 */
>     450 	if (WARN_ON_ONCE(metadata.event_len > count))
>     451 		goto out_close_fd;
>     452 
>     453 	if (copy_to_user(buf, &metadata, FAN_EVENT_METADATA_LEN))
>     454 		goto out_close_fd;
>                         ^^^^^^^^^^^^^^^^^
> This is problematic
> 
>     455 
>     456 	buf += FAN_EVENT_METADATA_LEN;
>     457 	count -= FAN_EVENT_METADATA_LEN;
>     458 
>     459 	if (fanotify_is_perm_event(event->mask))
>     460 		FANOTIFY_PERM(event)->fd = fd;
>     461 
>     462 	if (f)
>                    ^^^
> 
>     463 		fd_install(fd, f);
>     464 
>     465 	/* Event info records order is: dir fid + name, child fid */
>     466 	if (fanotify_event_dir_fh_len(event)) {
>     467 		info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
>     468 					     FAN_EVENT_INFO_TYPE_DFID;
>     469 		ret = copy_info_to_user(fanotify_event_fsid(event),
>     470 					fanotify_info_dir_fh(info),
>     471 					info_type, fanotify_info_name(info),
>     472 					info->name_len, buf, count);
>     473 		if (ret < 0)
>     474 			goto out_close_fd;
>     475 
>     476 		buf += ret;
>     477 		count -= ret;
>     478 	}
>     479 
>     480 	if (fanotify_event_object_fh_len(event)) {
>     481 		const char *dot = NULL;
>     482 		int dot_len = 0;
>     483 
>     484 		if (fid_mode == FAN_REPORT_FID || info_type) {
>     485 			/*
>     486 			 * With only group flag FAN_REPORT_FID only type FID is
>     487 			 * reported. Second info record type is always FID.
>     488 			 */
>     489 			info_type = FAN_EVENT_INFO_TYPE_FID;
>     490 		} else if ((fid_mode & FAN_REPORT_NAME) &&
>     491 			   (event->mask & FAN_ONDIR)) {
>     492 			/*
>     493 			 * With group flag FAN_REPORT_NAME, if name was not
>     494 			 * recorded in an event on a directory, report the
>     495 			 * name "." with info type DFID_NAME.
>     496 			 */
>     497 			info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
>     498 			dot = ".";
>     499 			dot_len = 1;
>     500 		} else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
>     501 			   (event->mask & FAN_ONDIR)) {
>     502 			/*
>     503 			 * With group flag FAN_REPORT_DIR_FID, a single info
>     504 			 * record has type DFID for directory entry modification
>     505 			 * event and for event on a directory.
>     506 			 */
>     507 			info_type = FAN_EVENT_INFO_TYPE_DFID;
>     508 		} else {
>     509 			/*
>     510 			 * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
>     511 			 * a single info record has type FID for event on a
>     512 			 * non-directory, when there is no directory to report.
>     513 			 * For example, on FAN_DELETE_SELF event.
>     514 			 */
>     515 			info_type = FAN_EVENT_INFO_TYPE_FID;
>     516 		}
>     517 
>     518 		ret = copy_info_to_user(fanotify_event_fsid(event),
>     519 					fanotify_event_object_fh(event),
>     520 					info_type, dot, dot_len, buf, count);
>     521 		if (ret < 0)
>     522 			goto out_close_fd;
>                                 ^^^^^^^^^^^^^^^^^
> 
> 
>     523 
>     524 		buf += ret;
>     525 		count -= ret;
>     526 	}
>     527 
>     528 	return metadata.event_len;
>     529 
>     530 out_close_fd:
>     531 	if (fd != FAN_NOFD) {
>     532 		put_unused_fd(fd);
> --> 533 		fput(f);
>                         ^^^^^^^
> This leads to a NULL dereference

Sure would, however if the intial else path is taken above skipping
create_fd() then "fd" would remain set to FAN_NOFD and "f" would remain set
to NULL, then this branch would not be taken and thus not leading to a NULL
dereference?

To make things clearer, avoid any future confusion and possibly tripping
over such a bug, perhaps it'd be better to split up the fput(f) call into a
separate branch outside of the current conditional, simply i.e.

...

if (f)
	fput(f);

...

Thoughts?

>     534 	}
>     535 	return ret;
>     536 }

/M

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] fanotify: fix copy_event_to_user() fid error clean up
  2021-07-21 22:38 ` Matthew Bobrowski
@ 2021-07-22  9:01   ` Amir Goldstein
  2021-07-22  9:31     ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Amir Goldstein @ 2021-07-22  9:01 UTC (permalink / raw)
  To: Matthew Bobrowski; +Cc: Dan Carpenter, linux-fsdevel

On Thu, Jul 22, 2021 at 1:39 AM Matthew Bobrowski <repnop@google.com> wrote:
>
> On Wed, Jul 21, 2021 at 05:54:07AM -0700, Dan Carpenter wrote:
> > Hello Matthew Bobrowski,
> >
> > The patch f644bc449b37: "fanotify: fix copy_event_to_user() fid error
> > clean up" from Jun 11, 2021, leads to the following static checker
> > warning:
> >
> >       fs/notify/fanotify/fanotify_user.c:533 copy_event_to_user()
> >       error: we previously assumed 'f' could be null (see line 462)
>
> I've made a couple comments below. What am I missing?
>
> > fs/notify/fanotify/fanotify_user.c
> >     401 static ssize_t copy_event_to_user(struct fsnotify_group *group,
> >     402                                 struct fanotify_event *event,
> >     403                                 char __user *buf, size_t count)
> >     404 {
> >     405       struct fanotify_event_metadata metadata;
> >     406       struct path *path = fanotify_event_path(event);
> >     407       struct fanotify_info *info = fanotify_event_info(event);
> >     408       unsigned int fid_mode = FAN_GROUP_FLAG(group, FANOTIFY_FID_BITS);
> >     409       struct file *f = NULL;
> >     410       int ret, fd = FAN_NOFD;
> >     411       int info_type = 0;
> >     412
> >     413       pr_debug("%s: group=%p event=%p\n", __func__, group, event);
> >     414
> >     415       metadata.event_len = FAN_EVENT_METADATA_LEN +
> >     416                               fanotify_event_info_len(fid_mode, event);
> >     417       metadata.metadata_len = FAN_EVENT_METADATA_LEN;
> >     418       metadata.vers = FANOTIFY_METADATA_VERSION;
> >     419       metadata.reserved = 0;
> >     420       metadata.mask = event->mask & FANOTIFY_OUTGOING_EVENTS;
> >     421       metadata.pid = pid_vnr(event->pid);
> >     422       /*
> >     423        * For an unprivileged listener, event->pid can be used to identify the
> >     424        * events generated by the listener process itself, without disclosing
> >     425        * the pids of other processes.
> >     426        */
> >     427       if (FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
> >     428           task_tgid(current) != event->pid)
> >     429               metadata.pid = 0;
> >     430
> >     431       /*
> >     432        * For now, fid mode is required for an unprivileged listener and
> >     433        * fid mode does not report fd in events.  Keep this check anyway
> >     434        * for safety in case fid mode requirement is relaxed in the future
> >     435        * to allow unprivileged listener to get events with no fd and no fid.
> >     436        */
> >     437       if (!FAN_GROUP_FLAG(group, FANOTIFY_UNPRIV) &&
> >     438           path && path->mnt && path->dentry) {
> >     439               fd = create_fd(group, path, &f);
> >     440               if (fd < 0)
> >     441                       return fd;
> >     442       }
> >
> > "f" is NULL on the else path
>
> Uh ha, although "fd" on the else path also remains set to the initial value
> of FAN_NOFD, right?
>
> >     443       metadata.fd = fd;
> >     444
> >     445       ret = -EFAULT;
> >     446       /*
> >     447        * Sanity check copy size in case get_one_event() and
> >     448        * event_len sizes ever get out of sync.
> >     449        */
> >     450       if (WARN_ON_ONCE(metadata.event_len > count))
> >     451               goto out_close_fd;
> >     452
> >     453       if (copy_to_user(buf, &metadata, FAN_EVENT_METADATA_LEN))
> >     454               goto out_close_fd;
> >                         ^^^^^^^^^^^^^^^^^
> > This is problematic
> >
> >     455
> >     456       buf += FAN_EVENT_METADATA_LEN;
> >     457       count -= FAN_EVENT_METADATA_LEN;
> >     458
> >     459       if (fanotify_is_perm_event(event->mask))
> >     460               FANOTIFY_PERM(event)->fd = fd;
> >     461
> >     462       if (f)
> >                    ^^^
> >
> >     463               fd_install(fd, f);
> >     464
> >     465       /* Event info records order is: dir fid + name, child fid */
> >     466       if (fanotify_event_dir_fh_len(event)) {
> >     467               info_type = info->name_len ? FAN_EVENT_INFO_TYPE_DFID_NAME :
> >     468                                            FAN_EVENT_INFO_TYPE_DFID;
> >     469               ret = copy_info_to_user(fanotify_event_fsid(event),
> >     470                                       fanotify_info_dir_fh(info),
> >     471                                       info_type, fanotify_info_name(info),
> >     472                                       info->name_len, buf, count);
> >     473               if (ret < 0)
> >     474                       goto out_close_fd;
> >     475
> >     476               buf += ret;
> >     477               count -= ret;
> >     478       }
> >     479
> >     480       if (fanotify_event_object_fh_len(event)) {
> >     481               const char *dot = NULL;
> >     482               int dot_len = 0;
> >     483
> >     484               if (fid_mode == FAN_REPORT_FID || info_type) {
> >     485                       /*
> >     486                        * With only group flag FAN_REPORT_FID only type FID is
> >     487                        * reported. Second info record type is always FID.
> >     488                        */
> >     489                       info_type = FAN_EVENT_INFO_TYPE_FID;
> >     490               } else if ((fid_mode & FAN_REPORT_NAME) &&
> >     491                          (event->mask & FAN_ONDIR)) {
> >     492                       /*
> >     493                        * With group flag FAN_REPORT_NAME, if name was not
> >     494                        * recorded in an event on a directory, report the
> >     495                        * name "." with info type DFID_NAME.
> >     496                        */
> >     497                       info_type = FAN_EVENT_INFO_TYPE_DFID_NAME;
> >     498                       dot = ".";
> >     499                       dot_len = 1;
> >     500               } else if ((event->mask & ALL_FSNOTIFY_DIRENT_EVENTS) ||
> >     501                          (event->mask & FAN_ONDIR)) {
> >     502                       /*
> >     503                        * With group flag FAN_REPORT_DIR_FID, a single info
> >     504                        * record has type DFID for directory entry modification
> >     505                        * event and for event on a directory.
> >     506                        */
> >     507                       info_type = FAN_EVENT_INFO_TYPE_DFID;
> >     508               } else {
> >     509                       /*
> >     510                        * With group flags FAN_REPORT_DIR_FID|FAN_REPORT_FID,
> >     511                        * a single info record has type FID for event on a
> >     512                        * non-directory, when there is no directory to report.
> >     513                        * For example, on FAN_DELETE_SELF event.
> >     514                        */
> >     515                       info_type = FAN_EVENT_INFO_TYPE_FID;
> >     516               }
> >     517
> >     518               ret = copy_info_to_user(fanotify_event_fsid(event),
> >     519                                       fanotify_event_object_fh(event),
> >     520                                       info_type, dot, dot_len, buf, count);
> >     521               if (ret < 0)
> >     522                       goto out_close_fd;
> >                                 ^^^^^^^^^^^^^^^^^
> >
> >
> >     523
> >     524               buf += ret;
> >     525               count -= ret;
> >     526       }
> >     527
> >     528       return metadata.event_len;
> >     529
> >     530 out_close_fd:
> >     531       if (fd != FAN_NOFD) {
> >     532               put_unused_fd(fd);
> > --> 533               fput(f);
> >                         ^^^^^^^
> > This leads to a NULL dereference
>
> Sure would, however if the intial else path is taken above skipping
> create_fd() then "fd" would remain set to FAN_NOFD and "f" would remain set
> to NULL, then this branch would not be taken and thus not leading to a NULL
> dereference?
>
> To make things clearer, avoid any future confusion and possibly tripping
> over such a bug, perhaps it'd be better to split up the fput(f) call into a
> separate branch outside of the current conditional, simply i.e.
>
> ...
>
> if (f)
>         fput(f);
>
> ...
>
> Thoughts?

smatch (apparently) does not know about the relation that f is non NULL
if (fd ==  FAN_NOFD) it needs to study create_fd() for that.

I suggest to move fd_install(fd, f); right after checking of return value
from create_fd() and without the if (f) condition.
That should make it clear for human and robots reading this function
that the cleanup in out_close_fd label is correct.

Thanks,
Amir.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] fanotify: fix copy_event_to_user() fid error clean up
  2021-07-22  9:01   ` Amir Goldstein
@ 2021-07-22  9:31     ` Dan Carpenter
  2021-07-22 14:01       ` Dan Carpenter
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-07-22  9:31 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Matthew Bobrowski, linux-fsdevel

On Thu, Jul 22, 2021 at 12:01:41PM +0300, Amir Goldstein wrote:
> On Thu, Jul 22, 2021 at 1:39 AM Matthew Bobrowski <repnop@google.com> wrote:
> > To make things clearer, avoid any future confusion and possibly tripping
> > over such a bug, perhaps it'd be better to split up the fput(f) call into a
> > separate branch outside of the current conditional, simply i.e.
> >
> > ...
> >
> > if (f)
> >         fput(f);
> >
> > ...
> >
> > Thoughts?
> 
> smatch (apparently) does not know about the relation that f is non NULL
> if (fd ==  FAN_NOFD) it needs to study create_fd() for that.
> 
> I suggest to move fd_install(fd, f); right after checking of return value
> from create_fd() and without the if (f) condition.
> That should make it clear for human and robots reading this function
> that the cleanup in out_close_fd label is correct.

Yeah.  I got "f" and "fd" mixed up in my head when I was reviewing this
code.  My bad.

Smatch is *supposed* to know about the relationship between those two.
The bug is actually that Smatch records in the database that create_fd()
always fails.  It's a serious bug, and I'm trying to investigate what's
going on and I'm sure that I will fix this before the end of the week.

No need to change the code just to work around a static checker bug.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] fanotify: fix copy_event_to_user() fid error clean up
  2021-07-22  9:31     ` Dan Carpenter
@ 2021-07-22 14:01       ` Dan Carpenter
  2021-07-22 23:13         ` Matthew Bobrowski
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2021-07-22 14:01 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Matthew Bobrowski, linux-fsdevel

On Thu, Jul 22, 2021 at 12:31:42PM +0300, Dan Carpenter wrote:
> Smatch is *supposed* to know about the relationship between those two.
> The bug is actually that Smatch records in the database that create_fd()
> always fails.  It's a serious bug, and I'm trying to investigate what's
> going on and I'm sure that I will fix this before the end of the week.

I'm testing a Smatch fix for this so hopefully it will pushed in a few
days.

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [bug report] fanotify: fix copy_event_to_user() fid error clean up
  2021-07-22 14:01       ` Dan Carpenter
@ 2021-07-22 23:13         ` Matthew Bobrowski
  0 siblings, 0 replies; 6+ messages in thread
From: Matthew Bobrowski @ 2021-07-22 23:13 UTC (permalink / raw)
  To: Dan Carpenter; +Cc: Amir Goldstein, linux-fsdevel

On Thu, Jul 22, 2021 at 05:01:03PM +0300, Dan Carpenter wrote:
> On Thu, Jul 22, 2021 at 12:31:42PM +0300, Dan Carpenter wrote:
> > Smatch is *supposed* to know about the relationship between those two.
> > The bug is actually that Smatch records in the database that create_fd()
> > always fails.  It's a serious bug, and I'm trying to investigate what's
> > going on and I'm sure that I will fix this before the end of the week.
> 
> I'm testing a Smatch fix for this so hopefully it will pushed in a few
> days.

Great!

Well, do let us know what the outcome is post running the Smatch tests
against the copy_event_to_user() function once again...

I do feel as though shuffling things around isn't necessary. Especially
considering the fact that what is current is correct and as you mentioned
this is a perfect oppurtunity to make the tooling better. :)

/M

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-07-22 23:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-21 12:54 [bug report] fanotify: fix copy_event_to_user() fid error clean up Dan Carpenter
2021-07-21 22:38 ` Matthew Bobrowski
2021-07-22  9:01   ` Amir Goldstein
2021-07-22  9:31     ` Dan Carpenter
2021-07-22 14:01       ` Dan Carpenter
2021-07-22 23:13         ` Matthew Bobrowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.