From: Amir Goldstein <amir73il@gmail.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: kbuild@lists.01.org, kbuild test robot <lkp@intel.com>,
kbuild-all@lists.01.org,
linux-kernel <linux-kernel@vger.kernel.org>,
Matthew Bobrowski <repnop@google.com>, Jan Kara <jack@suse.cz>
Subject: Re: [amir73il:fsnotify-fixes 2/2] fs/notify/fsnotify.c:540 fsnotify() warn: variable dereferenced before check 'dir1' (see line 499)
Date: Mon, 9 May 2022 14:48:36 +0300 [thread overview]
Message-ID: <CAOQ4uxiuF7qfUjmSHu1RgwcEhDvbU9nA6nFpMXCmrZ9qE4c4rw@mail.gmail.com> (raw)
In-Reply-To: <202205080346.m0fb3UXK-lkp@intel.com>
On Mon, May 9, 2022 at 1:09 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> tree: https://github.com/amir73il/linux fsnotify-fixes
> head: d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6
> commit: d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6 [2/2] fsnotify: send FS_RENAME to groups watching the moved inode
> config: s390-randconfig-m031-20220508 (https://download.01.org/0day-ci/archive/20220508/202205080346.m0fb3UXK-lkp@intel.com/config)
> compiler: s390-linux-gcc (GCC) 11.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> fs/notify/fsnotify.c:540 fsnotify() warn: variable dereferenced before check 'dir1' (see line 499)
>
> vim +/dir1 +540 fs/notify/fsnotify.c
>
> 40a100d3adc1ad Amir Goldstein 2020-07-22 475 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> 40a100d3adc1ad Amir Goldstein 2020-07-22 476 const struct qstr *file_name, struct inode *inode, u32 cookie)
> 90586523eb4b34 Eric Paris 2009-05-21 477 {
> b54cecf5e2293d Amir Goldstein 2020-06-07 478 const struct path *path = fsnotify_data_path(data, data_type);
> 29335033c574a1 Gabriel Krisman Bertazi 2021-10-25 479 struct super_block *sb = fsnotify_data_sb(data, data_type);
> 3427ce71554123 Miklos Szeredi 2017-10-30 480 struct fsnotify_iter_info iter_info = {};
> 60f7ed8c7c4d06 Amir Goldstein 2018-09-01 481 struct mount *mnt = NULL;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 482 struct inode *dir1, *dir2;
> e54183fa7047c1 Amir Goldstein 2021-11-29 483 struct dentry *moved;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 484 int dir1_type = 0;
> 9385a84d7e1f65 Jan Kara 2016-11-10 485 int ret = 0;
> 71d734103edfa2 Mel Gorman 2020-07-08 486 __u32 test_mask, marks_mask;
> 90586523eb4b34 Eric Paris 2009-05-21 487
> 71d734103edfa2 Mel Gorman 2020-07-08 488 if (path)
> aa93bdc5500cc9 Amir Goldstein 2020-03-19 489 mnt = real_mount(path->mnt);
> 3a9fb89f4cd04c Eric Paris 2009-12-17 490
> 40a100d3adc1ad Amir Goldstein 2020-07-22 491 if (!inode) {
> 40a100d3adc1ad Amir Goldstein 2020-07-22 492 /* Dirent event - report on TYPE_INODE to dir */
> 40a100d3adc1ad Amir Goldstein 2020-07-22 493 inode = dir;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 494 } else if (mask & FS_RENAME) {
> d25f3ce8da49ce Amir Goldstein 2022-05-07 495 /* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
> e54183fa7047c1 Amir Goldstein 2021-11-29 496 moved = fsnotify_data_dentry(data, data_type);
> d25f3ce8da49ce Amir Goldstein 2022-05-07 497 dir1 = moved->d_parent->d_inode;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 498 dir2 = dir;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 @499 if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
> ^^^^^^^^^^^^^^^^^^^^^^
> Dereference
>
> d25f3ce8da49ce Amir Goldstein 2022-05-07 500 dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 501 /*
> d25f3ce8da49ce Amir Goldstein 2022-05-07 502 * Send FS_RENAME to groups watching the moved inode itself
> d25f3ce8da49ce Amir Goldstein 2022-05-07 503 * only if the moved inode is a non-dir.
> d25f3ce8da49ce Amir Goldstein 2022-05-07 504 * Sending FS_RENAME to a moved watched directory would be
> d25f3ce8da49ce Amir Goldstein 2022-05-07 505 * confusing and FS_MOVE_SELF provided enough information to
> d25f3ce8da49ce Amir Goldstein 2022-05-07 506 * track the movements of a watched directory.
> d25f3ce8da49ce Amir Goldstein 2022-05-07 507 */
> d25f3ce8da49ce Amir Goldstein 2022-05-07 508 if (mask & FS_ISDIR)
> d25f3ce8da49ce Amir Goldstein 2022-05-07 509 inode = NULL;
> 40a100d3adc1ad Amir Goldstein 2020-07-22 510 } else if (mask & FS_EVENT_ON_CHILD) {
> 40a100d3adc1ad Amir Goldstein 2020-07-22 511 /*
> fecc4559780d52 Amir Goldstein 2020-12-02 512 * Event on child - report on TYPE_PARENT to dir if it is
> fecc4559780d52 Amir Goldstein 2020-12-02 513 * watching children and on TYPE_INODE to child.
> 40a100d3adc1ad Amir Goldstein 2020-07-22 514 */
> d25f3ce8da49ce Amir Goldstein 2022-05-07 515 dir1 = dir;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 516 dir2 = NULL;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 517 if (dir1->i_fsnotify_marks)
> ^^^^^^^^^^^^^^^^^^^^^^
> Dereference
>
> d25f3ce8da49ce Amir Goldstein 2022-05-07 518 dir1_type = FSNOTIFY_ITER_TYPE_PARENT;
> 40a100d3adc1ad Amir Goldstein 2020-07-22 519 }
> 497b0c5a7c0688 Amir Goldstein 2020-07-16 520
> 7c49b8616460eb Dave Hansen 2015-09-04 521 /*
> 7c49b8616460eb Dave Hansen 2015-09-04 522 * Optimization: srcu_read_lock() has a memory barrier which can
> 7c49b8616460eb Dave Hansen 2015-09-04 523 * be expensive. It protects walking the *_fsnotify_marks lists.
> 7c49b8616460eb Dave Hansen 2015-09-04 524 * However, if we do not walk the lists, we do not have to do
> 7c49b8616460eb Dave Hansen 2015-09-04 525 * SRCU because we have no references to any objects and do not
> 7c49b8616460eb Dave Hansen 2015-09-04 526 * need SRCU to keep them "alive".
> 7c49b8616460eb Dave Hansen 2015-09-04 527 */
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 528 if (!sb->s_fsnotify_marks &&
> 497b0c5a7c0688 Amir Goldstein 2020-07-16 529 (!mnt || !mnt->mnt_fsnotify_marks) &&
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 530 (!inode || !inode->i_fsnotify_marks) &&
> d25f3ce8da49ce Amir Goldstein 2022-05-07 531 !dir1_type)
> 7c49b8616460eb Dave Hansen 2015-09-04 532 return 0;
> 71d734103edfa2 Mel Gorman 2020-07-08 533
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 534 marks_mask = sb->s_fsnotify_mask;
> 71d734103edfa2 Mel Gorman 2020-07-08 535 if (mnt)
> 71d734103edfa2 Mel Gorman 2020-07-08 536 marks_mask |= mnt->mnt_fsnotify_mask;
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 537 if (inode)
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 538 marks_mask |= inode->i_fsnotify_mask;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 539 if (dir1_type) {
> d25f3ce8da49ce Amir Goldstein 2022-05-07 @540 if (dir1)
>
> If "dir1_type" is set then we have already dereferenced "dir1". I guess
> this unnecessary NULL check is something that probably wouldn't bother
> a human reader too much...
True !!dir1_type means !!dir1.
We could get rid of if (dir1)
but I should note that I did not post this patch for review, this is an untested
patch that I pushed to a branch on my private github.
It is very generous of kbuild testbot to test any branch I push to my
private github,
but as much as I can recall, I never asked for it...
Thanks,
Amir.
WARNING: multiple messages have this Message-ID (diff)
From: Amir Goldstein <amir73il@gmail.com>
To: kbuild-all@lists.01.org
Subject: Re: [amir73il:fsnotify-fixes 2/2] fs/notify/fsnotify.c:540 fsnotify() warn: variable dereferenced before check 'dir1' (see line 499)
Date: Mon, 09 May 2022 14:48:36 +0300 [thread overview]
Message-ID: <CAOQ4uxiuF7qfUjmSHu1RgwcEhDvbU9nA6nFpMXCmrZ9qE4c4rw@mail.gmail.com> (raw)
In-Reply-To: <202205080346.m0fb3UXK-lkp@intel.com>
[-- Attachment #1: Type: text/plain, Size: 8245 bytes --]
On Mon, May 9, 2022 at 1:09 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> tree: https://github.com/amir73il/linux fsnotify-fixes
> head: d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6
> commit: d25f3ce8da49ce1a3b0a0621f0bf7b1d6ba2dad6 [2/2] fsnotify: send FS_RENAME to groups watching the moved inode
> config: s390-randconfig-m031-20220508 (https://download.01.org/0day-ci/archive/20220508/202205080346.m0fb3UXK-lkp(a)intel.com/config)
> compiler: s390-linux-gcc (GCC) 11.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> smatch warnings:
> fs/notify/fsnotify.c:540 fsnotify() warn: variable dereferenced before check 'dir1' (see line 499)
>
> vim +/dir1 +540 fs/notify/fsnotify.c
>
> 40a100d3adc1ad Amir Goldstein 2020-07-22 475 int fsnotify(__u32 mask, const void *data, int data_type, struct inode *dir,
> 40a100d3adc1ad Amir Goldstein 2020-07-22 476 const struct qstr *file_name, struct inode *inode, u32 cookie)
> 90586523eb4b34 Eric Paris 2009-05-21 477 {
> b54cecf5e2293d Amir Goldstein 2020-06-07 478 const struct path *path = fsnotify_data_path(data, data_type);
> 29335033c574a1 Gabriel Krisman Bertazi 2021-10-25 479 struct super_block *sb = fsnotify_data_sb(data, data_type);
> 3427ce71554123 Miklos Szeredi 2017-10-30 480 struct fsnotify_iter_info iter_info = {};
> 60f7ed8c7c4d06 Amir Goldstein 2018-09-01 481 struct mount *mnt = NULL;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 482 struct inode *dir1, *dir2;
> e54183fa7047c1 Amir Goldstein 2021-11-29 483 struct dentry *moved;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 484 int dir1_type = 0;
> 9385a84d7e1f65 Jan Kara 2016-11-10 485 int ret = 0;
> 71d734103edfa2 Mel Gorman 2020-07-08 486 __u32 test_mask, marks_mask;
> 90586523eb4b34 Eric Paris 2009-05-21 487
> 71d734103edfa2 Mel Gorman 2020-07-08 488 if (path)
> aa93bdc5500cc9 Amir Goldstein 2020-03-19 489 mnt = real_mount(path->mnt);
> 3a9fb89f4cd04c Eric Paris 2009-12-17 490
> 40a100d3adc1ad Amir Goldstein 2020-07-22 491 if (!inode) {
> 40a100d3adc1ad Amir Goldstein 2020-07-22 492 /* Dirent event - report on TYPE_INODE to dir */
> 40a100d3adc1ad Amir Goldstein 2020-07-22 493 inode = dir;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 494 } else if (mask & FS_RENAME) {
> d25f3ce8da49ce Amir Goldstein 2022-05-07 495 /* For FS_RENAME, dir1 is old_dir and dir2 is new_dir */
> e54183fa7047c1 Amir Goldstein 2021-11-29 496 moved = fsnotify_data_dentry(data, data_type);
> d25f3ce8da49ce Amir Goldstein 2022-05-07 497 dir1 = moved->d_parent->d_inode;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 498 dir2 = dir;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 @499 if (dir1->i_fsnotify_marks || dir2->i_fsnotify_marks)
> ^^^^^^^^^^^^^^^^^^^^^^
> Dereference
>
> d25f3ce8da49ce Amir Goldstein 2022-05-07 500 dir1_type = FSNOTIFY_ITER_TYPE_OLD_DIR;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 501 /*
> d25f3ce8da49ce Amir Goldstein 2022-05-07 502 * Send FS_RENAME to groups watching the moved inode itself
> d25f3ce8da49ce Amir Goldstein 2022-05-07 503 * only if the moved inode is a non-dir.
> d25f3ce8da49ce Amir Goldstein 2022-05-07 504 * Sending FS_RENAME to a moved watched directory would be
> d25f3ce8da49ce Amir Goldstein 2022-05-07 505 * confusing and FS_MOVE_SELF provided enough information to
> d25f3ce8da49ce Amir Goldstein 2022-05-07 506 * track the movements of a watched directory.
> d25f3ce8da49ce Amir Goldstein 2022-05-07 507 */
> d25f3ce8da49ce Amir Goldstein 2022-05-07 508 if (mask & FS_ISDIR)
> d25f3ce8da49ce Amir Goldstein 2022-05-07 509 inode = NULL;
> 40a100d3adc1ad Amir Goldstein 2020-07-22 510 } else if (mask & FS_EVENT_ON_CHILD) {
> 40a100d3adc1ad Amir Goldstein 2020-07-22 511 /*
> fecc4559780d52 Amir Goldstein 2020-12-02 512 * Event on child - report on TYPE_PARENT to dir if it is
> fecc4559780d52 Amir Goldstein 2020-12-02 513 * watching children and on TYPE_INODE to child.
> 40a100d3adc1ad Amir Goldstein 2020-07-22 514 */
> d25f3ce8da49ce Amir Goldstein 2022-05-07 515 dir1 = dir;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 516 dir2 = NULL;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 517 if (dir1->i_fsnotify_marks)
> ^^^^^^^^^^^^^^^^^^^^^^
> Dereference
>
> d25f3ce8da49ce Amir Goldstein 2022-05-07 518 dir1_type = FSNOTIFY_ITER_TYPE_PARENT;
> 40a100d3adc1ad Amir Goldstein 2020-07-22 519 }
> 497b0c5a7c0688 Amir Goldstein 2020-07-16 520
> 7c49b8616460eb Dave Hansen 2015-09-04 521 /*
> 7c49b8616460eb Dave Hansen 2015-09-04 522 * Optimization: srcu_read_lock() has a memory barrier which can
> 7c49b8616460eb Dave Hansen 2015-09-04 523 * be expensive. It protects walking the *_fsnotify_marks lists.
> 7c49b8616460eb Dave Hansen 2015-09-04 524 * However, if we do not walk the lists, we do not have to do
> 7c49b8616460eb Dave Hansen 2015-09-04 525 * SRCU because we have no references to any objects and do not
> 7c49b8616460eb Dave Hansen 2015-09-04 526 * need SRCU to keep them "alive".
> 7c49b8616460eb Dave Hansen 2015-09-04 527 */
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 528 if (!sb->s_fsnotify_marks &&
> 497b0c5a7c0688 Amir Goldstein 2020-07-16 529 (!mnt || !mnt->mnt_fsnotify_marks) &&
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 530 (!inode || !inode->i_fsnotify_marks) &&
> d25f3ce8da49ce Amir Goldstein 2022-05-07 531 !dir1_type)
> 7c49b8616460eb Dave Hansen 2015-09-04 532 return 0;
> 71d734103edfa2 Mel Gorman 2020-07-08 533
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 534 marks_mask = sb->s_fsnotify_mask;
> 71d734103edfa2 Mel Gorman 2020-07-08 535 if (mnt)
> 71d734103edfa2 Mel Gorman 2020-07-08 536 marks_mask |= mnt->mnt_fsnotify_mask;
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 537 if (inode)
> 9b93f33105f5f9 Amir Goldstein 2020-07-16 538 marks_mask |= inode->i_fsnotify_mask;
> d25f3ce8da49ce Amir Goldstein 2022-05-07 539 if (dir1_type) {
> d25f3ce8da49ce Amir Goldstein 2022-05-07 @540 if (dir1)
>
> If "dir1_type" is set then we have already dereferenced "dir1". I guess
> this unnecessary NULL check is something that probably wouldn't bother
> a human reader too much...
True !!dir1_type means !!dir1.
We could get rid of if (dir1)
but I should note that I did not post this patch for review, this is an untested
patch that I pushed to a branch on my private github.
It is very generous of kbuild testbot to test any branch I push to my
private github,
but as much as I can recall, I never asked for it...
Thanks,
Amir.
next prev parent reply other threads:[~2022-05-09 11:49 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-07 19:26 [amir73il:fsnotify-fixes 2/2] fs/notify/fsnotify.c:540 fsnotify() warn: variable dereferenced before check 'dir1' (see line 499) kernel test robot
2022-05-09 10:08 ` Dan Carpenter
2022-05-09 10:08 ` Dan Carpenter
2022-05-09 11:48 ` Amir Goldstein [this message]
2022-05-09 11:48 ` Amir Goldstein
2022-05-12 9:32 ` [kbuild-all] " Chen, Rong A
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxiuF7qfUjmSHu1RgwcEhDvbU9nA6nFpMXCmrZ9qE4c4rw@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=jack@suse.cz \
--cc=kbuild-all@lists.01.org \
--cc=kbuild@lists.01.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
--cc=repnop@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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.