* re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces
@ 2013-09-02 15:20 Dan Carpenter
2013-09-02 20:30 ` Eric W. Biederman
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2013-09-02 15:20 UTC (permalink / raw)
To: ebiederm; +Cc: linux-fsdevel
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:
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces
2013-09-02 15:20 vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces Dan Carpenter
@ 2013-09-02 20:30 ` Eric W. Biederman
2013-09-02 20:40 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Eric W. Biederman @ 2013-09-02 20:30 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-fsdevel
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces
2013-09-02 20:30 ` Eric W. Biederman
@ 2013-09-02 20:40 ` Dan Carpenter
2013-09-02 21:52 ` Eric W. Biederman
0 siblings, 1 reply; 5+ messages in thread
From: Dan Carpenter @ 2013-09-02 20:40 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: linux-fsdevel
On Mon, Sep 02, 2013 at 01:30:00PM -0700, Eric W. Biederman wrote:
> 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.
>
Why not make the test:
while (1) { ...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces
2013-09-02 20:40 ` Dan Carpenter
@ 2013-09-02 21:52 ` Eric W. Biederman
2013-09-02 23:10 ` Dan Carpenter
0 siblings, 1 reply; 5+ messages in thread
From: Eric W. Biederman @ 2013-09-02 21:52 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-fsdevel
Dan Carpenter <dan.carpenter@oracle.com> writes:
> On Mon, Sep 02, 2013 at 01:30:00PM -0700, Eric W. Biederman wrote:
>> 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.
>>
>
> Why not make the test:
>
> while (1) { ...
I really don't understand what you are getting at, and it feels
like this conversation is descending into inanity fast.
The way this work is.
p = old
q = new
Where new is a copy of old.
p == NULL means that we have completed the traversal.
The entire point of the loop is to find corresponding entities in new
and old. Because in a few correr cases I slice off leaf mounts that
are mounted on top of files I need an extra loop, and I made that with
a very small code change.
So yes it does look like the case that except for the first iteration
the test at the top of the while loop is now redundant, because of the
if (!q) break; that I added.
I think any greater change to the loop would have obscured the change
that I made a great deal more.
I think all of this talking about the syntactical elements of the code
instead of brining this up to the level of human beings and talking
about what is going on is extremely dangerous way to look at code.
Especially given that C does not allow everything of importance about
the code to be expressed.
Eric
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces
2013-09-02 21:52 ` Eric W. Biederman
@ 2013-09-02 23:10 ` Dan Carpenter
0 siblings, 0 replies; 5+ messages in thread
From: Dan Carpenter @ 2013-09-02 23:10 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: linux-fsdevel
On Mon, Sep 02, 2013 at 02:52:20PM -0700, Eric W. Biederman wrote:
> So yes it does look like the case that except for the first iteration
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> the test at the top of the while loop is now redundant, because of the
> if (!q) break; that I added.
Ah... I see the confusion now.
The test is redundant on the first iteration as well. The test is
entirely redundant. That's the point.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-09-02 23:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-02 15:20 vfs: Don't copy mount bind mounts of /proc/<pid>/ns/mnt between namespaces Dan Carpenter
2013-09-02 20:30 ` Eric W. Biederman
2013-09-02 20:40 ` Dan Carpenter
2013-09-02 21:52 ` Eric W. Biederman
2013-09-02 23:10 ` Dan Carpenter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).