* [bug report] fs: Teach path_connected to handle nfs filesystems with multiple roots.
@ 2018-05-03 12:42 Dan Carpenter
2018-05-03 15:21 ` Eric W. Biederman
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2018-05-03 12:42 UTC (permalink / raw)
To: ebiederm; +Cc: linux-nfs
Hello Eric W. Biederman,
The patch 95dd77580ccd: "fs: Teach path_connected to handle nfs
filesystems with multiple roots." from Mar 14, 2018, leads to the
following static checker warning:
fs/nfs/super.c:2634 nfs_fs_mount_common()
error: potential NULL dereference 'server'.
fs/nfs/super.c
2598 if (server->flags & NFS_MOUNT_UNSHARED)
2599 compare_super = NULL;
2600
2601 /* -o noac implies -o sync */
2602 if (server->flags & NFS_MOUNT_NOAC)
2603 sb_mntdata.mntflags |= SB_SYNCHRONOUS;
2604
2605 if (mount_info->cloned != NULL && mount_info->cloned->sb != NULL)
2606 if (mount_info->cloned->sb->s_flags & SB_SYNCHRONOUS)
2607 sb_mntdata.mntflags |= SB_SYNCHRONOUS;
2608
2609 /* Get a superblock - note that we may end up sharing one that already exists */
2610 s = sget(nfs_mod->nfs_fs, compare_super, nfs_set_super, flags, &sb_mntdata);
2611 if (IS_ERR(s)) {
2612 mntroot = ERR_CAST(s);
2613 goto out_err_nosb;
2614 }
2615
2616 if (s->s_fs_info != server) {
2617 nfs_free_server(server);
2618 server = NULL;
^^^^^^^^^^^^^
We set server to NULL here. Smatch and I don't know the value of
s->s_root at this point, but maybe it can't be NULL here? If so, that's
fine then.
2619 } else {
2620 error = super_setup_bdi_name(s, "%u:%u", MAJOR(server->s_dev),
2621 MINOR(server->s_dev));
2622 if (error) {
2623 mntroot = ERR_PTR(error);
2624 goto error_splat_super;
2625 }
2626 s->s_bdi->ra_pages = server->rpages * NFS_MAX_READAHEAD;
2627 server->super = s;
2628 }
2629
2630 if (!s->s_root) {
2631 /* initial superblock/root creation */
2632 mount_info->fill_super(s, mount_info);
2633 nfs_get_cache_cookie(s, mount_info->parsed, mount_info->cloned);
2634 if (!(server->flags & NFS_MOUNT_UNSHARED))
^^^^^^^^^^^^^
Potential NULL dereference.
2635 s->s_iflags |= SB_I_MULTIROOT;
2636 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] fs: Teach path_connected to handle nfs filesystems with multiple roots.
2018-05-03 12:42 [bug report] fs: Teach path_connected to handle nfs filesystems with multiple roots Dan Carpenter
@ 2018-05-03 15:21 ` Eric W. Biederman
0 siblings, 0 replies; 2+ messages in thread
From: Eric W. Biederman @ 2018-05-03 15:21 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-nfs
Dan Carpenter <dan.carpenter@oracle.com> writes:
> Hello Eric W. Biederman,
>
> The patch 95dd77580ccd: "fs: Teach path_connected to handle nfs
> filesystems with multiple roots." from Mar 14, 2018, leads to the
> following static checker warning:
>
> fs/nfs/super.c:2634 nfs_fs_mount_common()
> error: potential NULL dereference 'server'.
>
> fs/nfs/super.c
> 2598 if (server->flags & NFS_MOUNT_UNSHARED)
> 2599 compare_super = NULL;
> 2600
> 2601 /* -o noac implies -o sync */
> 2602 if (server->flags & NFS_MOUNT_NOAC)
> 2603 sb_mntdata.mntflags |= SB_SYNCHRONOUS;
> 2604
> 2605 if (mount_info->cloned != NULL && mount_info->cloned->sb != NULL)
> 2606 if (mount_info->cloned->sb->s_flags & SB_SYNCHRONOUS)
> 2607 sb_mntdata.mntflags |= SB_SYNCHRONOUS;
> 2608
> 2609 /* Get a superblock - note that we may end up sharing one that already exists */
> 2610 s = sget(nfs_mod->nfs_fs, compare_super, nfs_set_super, flags, &sb_mntdata);
> 2611 if (IS_ERR(s)) {
> 2612 mntroot = ERR_CAST(s);
> 2613 goto out_err_nosb;
> 2614 }
> 2615
> 2616 if (s->s_fs_info != server) {
> 2617 nfs_free_server(server);
> 2618 server = NULL;
> ^^^^^^^^^^^^^
> We set server to NULL here. Smatch and I don't know the value of
> s->s_root at this point, but maybe it can't be NULL here? If so, that's
> fine then.
>
> 2619 } else {
> 2620 error = super_setup_bdi_name(s, "%u:%u", MAJOR(server->s_dev),
> 2621 MINOR(server->s_dev));
> 2622 if (error) {
> 2623 mntroot = ERR_PTR(error);
> 2624 goto error_splat_super;
> 2625 }
> 2626 s->s_bdi->ra_pages = server->rpages * NFS_MAX_READAHEAD;
> 2627 server->super = s;
> 2628 }
> 2629
> 2630 if (!s->s_root) {
> 2631 /* initial superblock/root creation */
> 2632 mount_info->fill_super(s, mount_info);
> 2633 nfs_get_cache_cookie(s, mount_info->parsed, mount_info->cloned);
> 2634 if (!(server->flags & NFS_MOUNT_UNSHARED))
> ^^^^^^^^^^^^^
> Potential NULL dereference.
>
> 2635 s->s_iflags |= SB_I_MULTIROOT;
> 2636 }
It is not pretty but I believe in practice the code is fine.
!server implies sget returned a preexisting superblock.
!s->s_root implies !(s->s_flags & SB_BORN)
sget will not return a superblock without SB_BORN being set.
Further it is mount_fs our ``caller'' (via way of whatever calls
nfs_mount_common) that sets SB_BORN when .mount succeeds.
Eric
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2018-05-03 15:22 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-03 12:42 [bug report] fs: Teach path_connected to handle nfs filesystems with multiple roots Dan Carpenter
2018-05-03 15:21 ` Eric W. Biederman
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.