[PATCH] export kernel call get_task_comm().

[PATCH] export kernel call get_task_comm().

[james_p_freyensee]

From: J Freyensee <james_p_freyensee@linux.intel.com>

This allows drivers who call this function to be compiled modularly.
Otherwise, a driver who is interested in this type of functionality
has to implement their own get_task_comm() call, causing code
duplication in the Linux source tree.

Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
---
 fs/exec.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 8328beb..e1ac338 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1004,6 +1004,7 @@ char *get_task_comm(char *buf, struct task_struct *tsk)
 	task_unlock(tsk);
 	return buf;
 }
+EXPORT_SYMBOL_GPL(get_task_comm);
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
-- 
1.7.2.3

[PATCH] export kernel call get_task_comm().

[james_p_freyensee]

From: J Freyensee <james_p_freyensee@linux.intel.com>

This allows drivers who call this function to be compiled modularly.
Otherwise, a driver who is interested in this type of functionality
has to implement their own get_task_comm() call, causing code
duplication in the Linux source tree.

Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
---
 fs/exec.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 8328beb..e1ac338 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1004,6 +1004,7 @@ char *get_task_comm(char *buf, struct task_struct *tsk)
 	task_unlock(tsk);
 	return buf;
 }
+EXPORT_SYMBOL_GPL(get_task_comm);
 
 void set_task_comm(struct task_struct *tsk, char *buf)
 {
-- 
1.7.2.3

Re: [PATCH] export kernel call get_task_comm().

[David Rientjes]

On Fri, 22 Apr 2011, james_p_freyensee@linux.intel.com wrote:

> From: J Freyensee <james_p_freyensee@linux.intel.com>
> 
> This allows drivers who call this function to be compiled modularly.
> Otherwise, a driver who is interested in this type of functionality
> has to implement their own get_task_comm() call, causing code
> duplication in the Linux source tree.
> 
> Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>

Acked-by: David Rientjes <rientjes@google.com>

I still suggest that we implement finer-grained protection for tsk->comm 
through get_task_comm(), though, because it's going to be difficult to 
know whether task_lock(tsk) is held in all contexts we'll want to call it; 
task_lock(tsk) is used to protect many members of task_struct.

Re: [PATCH] export kernel call get_task_comm().

[Greg KH]

On Fri, Apr 22, 2011 at 03:35:44PM -0700, james_p_freyensee@linux.intel.com wrote:
> From: J Freyensee <james_p_freyensee@linux.intel.com>
> 
> This allows drivers who call this function to be compiled modularly.
> Otherwise, a driver who is interested in this type of functionality
> has to implement their own get_task_comm() call, causing code
> duplication in the Linux source tree.
> 
> Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>

I think the goal is for the cleanup to happen now, to justify the
addition of the exported symbol.  Without that, there is no need to
export the symbol now at all, as who knows when your driver will be
accepted.

Or, just wait and make it part of your driver patch series, like you did
before, no need to get it accepted now, right?

thanks,

greg k-h

Re: [PATCH] export kernel call get_task_comm().

[J Freyensee]

On Fri, 2011-04-22 at 15:35 -0700, David Rientjes wrote:
> On Fri, 22 Apr 2011, james_p_freyensee@linux.intel.com wrote:
> 
> > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > 
> > This allows drivers who call this function to be compiled modularly.
> > Otherwise, a driver who is interested in this type of functionality
> > has to implement their own get_task_comm() call, causing code
> > duplication in the Linux source tree.
> > 
> > Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
> 
> Acked-by: David Rientjes <rientjes@google.com>
> 
> I still suggest that we implement finer-grained protection for tsk->comm 
> through get_task_comm(), though, because it's going to be difficult to 
> know whether task_lock(tsk) is held in all contexts we'll want to call it; 
> task_lock(tsk) is used to protect many members of task_struct.

Okay, but how about accepting this as step 1, then investigate a finer
grained lock structure as step 2?

Re: [PATCH] export kernel call get_task_comm().

[J Freyensee]

On Fri, 2011-04-22 at 15:43 -0700, Greg KH wrote:
> On Fri, Apr 22, 2011 at 03:35:44PM -0700, james_p_freyensee@linux.intel.com wrote:
> > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > 
> > This allows drivers who call this function to be compiled modularly.
> > Otherwise, a driver who is interested in this type of functionality
> > has to implement their own get_task_comm() call, causing code
> > duplication in the Linux source tree.
> > 
> > Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
> 
> I think the goal is for the cleanup to happen now, to justify the
> addition of the exported symbol.  Without that, there is no need to
> export the symbol now at all, as who knows when your driver will be
> accepted.
> 
> Or, just wait and make it part of your driver patch series, like you did
> before, no need to get it accepted now, right?
>

Well, at some point a few people like Alan Cox and Arjan VdV would like
to see this work on it's way to Linus's tree.

I'll do whatever is best and easiest for you and will bring a close to
my submission attempts.  I can also just go into the Kconfig where the
pti driver is configured and just make the selection bool, yes or no,
and not make it an option to compile this modularly.  Then I'll drop
this patch all together.  This is the whole reason why I'm making this
change.  I don't have to have the pti driver as a module, just more
convenient.  And within the fs/exec.c it states reads to 'current->comm'
without a lock is safe.

> thanks,
> 
> greg k-h

Re: [PATCH] export kernel call get_task_comm().

[Greg KH]

On Fri, Apr 22, 2011 at 03:59:42PM -0700, J Freyensee wrote:
> On Fri, 2011-04-22 at 15:43 -0700, Greg KH wrote:
> > On Fri, Apr 22, 2011 at 03:35:44PM -0700, james_p_freyensee@linux.intel.com wrote:
> > > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > > 
> > > This allows drivers who call this function to be compiled modularly.
> > > Otherwise, a driver who is interested in this type of functionality
> > > has to implement their own get_task_comm() call, causing code
> > > duplication in the Linux source tree.
> > > 
> > > Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
> > 
> > I think the goal is for the cleanup to happen now, to justify the
> > addition of the exported symbol.  Without that, there is no need to
> > export the symbol now at all, as who knows when your driver will be
> > accepted.
> > 
> > Or, just wait and make it part of your driver patch series, like you did
> > before, no need to get it accepted now, right?
> >
> 
> Well, at some point a few people like Alan Cox and Arjan VdV would like
> to see this work on it's way to Linus's tree.

That's because that is the policy of your distro you are working with,
which has nothing to do with the kernel developers.

Again, if you get your driver accepted, I have no objection to this
export at all.  Just take the time and get your driver merged, it's that
simple.

thanks,

greg k-h

Re: [PATCH] export kernel call get_task_comm().

[J Freyensee]

On Fri, 2011-04-22 at 16:04 -0700, Greg KH wrote:
> On Fri, Apr 22, 2011 at 03:59:42PM -0700, J Freyensee wrote:
> > On Fri, 2011-04-22 at 15:43 -0700, Greg KH wrote:
> > > On Fri, Apr 22, 2011 at 03:35:44PM -0700, james_p_freyensee@linux.intel.com wrote:
> > > > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > > > 
> > > > This allows drivers who call this function to be compiled modularly.
> > > > Otherwise, a driver who is interested in this type of functionality
> > > > has to implement their own get_task_comm() call, causing code
> > > > duplication in the Linux source tree.
> > > > 
> > > > Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
> > > 
> > > I think the goal is for the cleanup to happen now, to justify the
> > > addition of the exported symbol.  Without that, there is no need to
> > > export the symbol now at all, as who knows when your driver will be
> > > accepted.
> > > 
> > > Or, just wait and make it part of your driver patch series, like you did
> > > before, no need to get it accepted now, right?
> > >
> > 
> > Well, at some point a few people like Alan Cox and Arjan VdV would like
> > to see this work on it's way to Linus's tree.
> 
> That's because that is the policy of your distro you are working with,
> which has nothing to do with the kernel developers.
> 
> Again, if you get your driver accepted, I have no objection to this
> export at all.  Just take the time and get your driver merged, it's that
> simple.

So I guess the best route is for me to make this patch with my driver
then?  I'm ready to re-submit those drivers again; I cleaned up all the
style issues pointed out by Randy.

> 
> thanks,
> 
> greg k-h

Re: [PATCH] export kernel call get_task_comm().

[Greg KH]

On Fri, Apr 22, 2011 at 04:08:16PM -0700, J Freyensee wrote:
> On Fri, 2011-04-22 at 16:04 -0700, Greg KH wrote:
> > On Fri, Apr 22, 2011 at 03:59:42PM -0700, J Freyensee wrote:
> > > On Fri, 2011-04-22 at 15:43 -0700, Greg KH wrote:
> > > > On Fri, Apr 22, 2011 at 03:35:44PM -0700, james_p_freyensee@linux.intel.com wrote:
> > > > > From: J Freyensee <james_p_freyensee@linux.intel.com>
> > > > > 
> > > > > This allows drivers who call this function to be compiled modularly.
> > > > > Otherwise, a driver who is interested in this type of functionality
> > > > > has to implement their own get_task_comm() call, causing code
> > > > > duplication in the Linux source tree.
> > > > > 
> > > > > Signed-off-by: J Freyensee <james_p_freyensee@linux.intel.com>
> > > > 
> > > > I think the goal is for the cleanup to happen now, to justify the
> > > > addition of the exported symbol.  Without that, there is no need to
> > > > export the symbol now at all, as who knows when your driver will be
> > > > accepted.
> > > > 
> > > > Or, just wait and make it part of your driver patch series, like you did
> > > > before, no need to get it accepted now, right?
> > > >
> > > 
> > > Well, at some point a few people like Alan Cox and Arjan VdV would like
> > > to see this work on it's way to Linus's tree.
> > 
> > That's because that is the policy of your distro you are working with,
> > which has nothing to do with the kernel developers.
> > 
> > Again, if you get your driver accepted, I have no objection to this
> > export at all.  Just take the time and get your driver merged, it's that
> > simple.
> 
> So I guess the best route is for me to make this patch with my driver
> then?  I'm ready to re-submit those drivers again; I cleaned up all the
> style issues pointed out by Randy.

Yes, make it part of that patch series.

thanks,

greg k-h

Re: [PATCH] export kernel call get_task_comm().

[David Rientjes]

On Fri, 22 Apr 2011, J Freyensee wrote:

> I'll do whatever is best and easiest for you and will bring a close to
> my submission attempts.  I can also just go into the Kconfig where the
> pti driver is configured and just make the selection bool, yes or no,
> and not make it an option to compile this modularly.  Then I'll drop
> this patch all together.  This is the whole reason why I'm making this
> change.  I don't have to have the pti driver as a module, just more
> convenient.  And within the fs/exec.c it states reads to 'current->comm'
> without a lock is safe.
> 

It's safe to read current->comm without holding task_lock(current), but it 
may be corrupted by a concurrent write via /proc/pid-of-current/comm, 
which could result in garbage where you'd expect the name of the thread.  
That doesn't sound very clean to me and adds more incentive to implement 
some finer-grained protection like a seqlock specifically for tsk->comm.

If /proc/pid/comm is really that important and we can't get away with the 
long-standing prctl(PR_SET_NAME), then you need to protect the string 
somehow and I'm quite surprised this wasn't required before writing other 
threads' comm was allowed.

If you can get away with task_lock(current) in your driver, then great, 
export the symbol and use it, but we have hundreds of racy reads to a 
thread's comm all over the kernel.