Re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces

[ebiederm@xmission.com (Eric W. Biederman)]

Dan Carpenter <dan.carpenter@oracle.com> writes:

> Hello Eric W. Biederman,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 4ce5d2b1a8fd: "vfs: Don't copy mount bind mounts of
> /proc/<pid>/ns/mnt between namespaces" from Mar 30, 2013, leads to
> the following Smatch complaint:

Semi-autoautomatic reply.  The test !q is enough to ensure p is valid
until p->mnt.mnt_root == q->mnt.mnt_root.

This has been verified through both testing and reading of the code.

Eric

> fs/namespace.c:2459 dup_mnt_ns()
> 	 warn: variable dereferenced before check 'p' (see line 2475)
>
> fs/namespace.c
>   2441          new = copy_tree(old, old->mnt.mnt_root, copy_flags);
>                                      ^^^^^^^^^^^^^^^^^
> Old is dereferenced.
>
>   2442          if (IS_ERR(new)) {
>   2443                  namespace_unlock();
>   2444                  free_mnt_ns(new_ns);
>   2445                  return ERR_CAST(new);
>   2446          }
>   2447          new_ns->root = new;
>   2448          br_write_lock(&vfsmount_lock);
>   2449          list_add_tail(&new_ns->list, &new->mnt_list);
>   2450          br_write_unlock(&vfsmount_lock);
>   2451  
>   2452          /*
>   2453           * Second pass: switch the tsk->fs->* elements and mark new vfsmounts
>   2454           * as belonging to new namespace.  We have already acquired a private
>   2455           * fs_struct, so tsk->fs->lock is not needed.
>   2456           */
>   2457          p = old;
>                 ^^^^^^^
>
> P is a valid pointer.
>
>   2458		q = new;
>   2459		while (p) {
>                       ^^^
> Existing check for "p".
>
>   2460			q->mnt_ns = new_ns;
>   2461			if (fs) {
>   2462				if (&p->mnt == fs->root.mnt) {
>   2463					fs->root.mnt = mntget(&q->mnt);
>   2464					rootmnt = &p->mnt;
>   2465				}
>   2466				if (&p->mnt == fs->pwd.mnt) {
>   2467					fs->pwd.mnt = mntget(&q->mnt);
>   2468					pwdmnt = &p->mnt;
>   2469				}
>   2470			}
>   2471			p = next_mnt(p, old);
>   2472			q = next_mnt(q, new);
>   2473			if (!q)
>   2474				break;
>   2475			while (p->mnt.mnt_root != q->mnt.mnt_root)
>                                ^^^^^^^^^^^^^^^
> We dereference "p".
>
>   2476				p = next_mnt(p, old);
>   2477		}
>
> regards,
> dan carpenter