From mboxrd@z Thu Jan  1 00:00:00 1970
From: ebiederm@xmission.com (Eric W. Biederman)
Subject: Re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces
Date: Mon, 02 Sep 2013 13:30:00 -0700
Message-ID: <874na3nf07.fsf@xmission.com>
References: <20130902140531.GA5450@elgon.mountain>
Mime-Version: 1.0
Content-Type: text/plain
Cc: linux-fsdevel@vger.kernel.org
To: Dan Carpenter <dan.carpenter@oracle.com>
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from out01.mta.xmission.com ([166.70.13.231]:40121 "EHLO
	out01.mta.xmission.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1757340Ab3IBUaM (ORCPT
	<rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 2 Sep 2013 16:30:12 -0400
In-Reply-To: <20130902140531.GA5450@elgon.mountain> (Dan Carpenter's message
	of "Mon, 2 Sep 2013 18:20:43 +0300")
Sender: linux-fsdevel-owner@vger.kernel.org
List-ID: <linux-fsdevel.vger.kernel.org>

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