Re: [Patch 5/8] generic netlink interface for delay accounting

[Andrew Morton]

Balbir Singh <balbir@xxxxxxxxxx> wrote:
>
> > The kmem_cache_free() can happen outside the lock.
> 
> 
> kmem_cache_free() and setting to NULL outside the lock is prone to
> race conditions. Consider the following scenario
> 
> A thread group T1 has exiting processes P1 and P2
> 
> P1 is exiting, finishes the delay accounting by calling taskstats_exit_pid()
> and gives up the mutex and calls kmem_cache_free(), but before it can set
> tsk->delays to NULL, we try to get statistics for the entire thread group.
> This task will show up in the thread group with a dangling tsk->delays.

Yes, the `tsk->delays = NULL;' needs to happen inside the lock.  But the
kmem_cache_free() does not.  It pointlessly increases the lock hold time.

> > > +	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> > > +		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> > > +		rc = fill_pid((pid_t)pid, NULL, &stats);
> > 
> > We shouldn't have a typecast here.  If it generates a warning then we need
> > to get in there and find out why.
> 
> The reason for a typecast is that pid is passed as a u32 from userspace.
> genetlink currently supports most unsigned types with little or no
> support for signed types. We exchange data as u32 and do the correct
> thing in the kernel. Would you like us to move away from this?
> 

I think it's best to avoid the cast unless it's actually needed to avoid a
warning or compile error, or to do special things with sign extension. 
Because casts clutter up the code and can hide real bugs.  In this case the
compiler should silently perform the conversion.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/