Andrew Morton wrote:
On Mon, 19 Feb 2007 16:39:33 +0530 Balbir Singh <[email protected]> wrote:
Andrew Morton wrote:
On Mon, 19 Feb 2007 16:07:44 +0530 Balbir Singh <[email protected]> wrote:
+void memctlr_mm_free(struct mm_struct *mm)
+{
+ kfree(mm->counter);
+}
+
+static inline void memctlr_mm_assign_container_direct(struct mm_struct *mm,
+ struct container *cont)
+{
+ write_lock(&mm->container_lock);
+ mm->container = cont;
+ write_unlock(&mm->container_lock);
+}
More weird locking here.
The container field of the mm_struct is protected by a read write spin lock.
That doesn't mean anything to me.
What would go wrong if the above locking was simply removed? And how does
the locking prevent that fault?
Some pages could charged to the wrong container. Apart from that I do not
see anything going bad (I'll double check that).
Argh. Please, think about this.
Sure, I will. I guess I am short circuiting my thinking process :-)
That locking *doesn't do anything*. Except for that one situation I
described: some other holder of the lock reads mm->container twice inside
the lock and requires that the value be the same both times (and that sort
of code should be converted to take a local copy, so this locking here can
be removed).
Yes, that makes sense.
+
+ read_lock(&mm->container_lock);
+ cont = mm->container;
+ read_unlock(&mm->container_lock);
+
+ if (!cont)
+ goto done;
And here. I mean, if there was a reason for taking the lock around that
read, then testing `cont' outside the lock just invalidated that reason.
We took a consistent snapshot of cont. It cannot change outside the lock,
we check the value outside. I am sure I missed something.
If it cannot change outside the lock then we don't need to take the lock!
We took a snapshot that we thought was consistent.
Consistent with what? That's a single-word read inside that lock.
Yes, that makes sense.
We check for the value
outside. I guess there is no harm, the worst thing that could happen
is wrong accounting during mm->container changes (when a task changes
container).
If container->lock is held when a task is removed from the
container then yes, `cont' here can refer to a container to which the task
no longer belongs.
More worrisome is the potential for use-after-free. What prevents the
pointer at mm->container from referring to freed memory after we're dropped
the lock?
The container cannot be freed unless all tasks holding references to it are
gone, that would ensure that all mm->containers are pointing elsewhere and
never to a stale value.
I hope my short-circuited brain got this right :-)
--
Warm Regards,
Balbir Singh
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
