linux-kernel - Re: [PATCH 1/1] Introduce Intel RAPL cooling device driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20130402171714.7309cdf2@chromoly>
Date:	Tue, 2 Apr 2013 17:17:14 -0700
From:	Jacob Pan <jacob.jun.pan@...ux.intel.com>
To:	Greg KH <gregkh@...uxfoundation.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Platform Driver <platform-driver-x86@...r.kernel.org>,
	Matthew Garrett <matthew.garrett@...ula.com>,
	Zhang Rui <rui.zhang@...el.com>,
	Rafael Wysocki <rafael.j.wysocki@...el.com>,
	Len Brown <len.brown@...el.com>,
	Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>,
	Arjan van de Ven <arjan@...ux.intel.com>
Subject: Re: [PATCH 1/1] Introduce Intel RAPL cooling device driver

On Tue, 2 Apr 2013 16:48:05 -0700
Greg KH <gregkh@...uxfoundation.org> wrote:

> On Tue, Apr 02, 2013 at 04:33:57PM -0700, Jacob Pan wrote:
> > On Tue, 2 Apr 2013 16:00:42 -0700
> > Greg KH <gregkh@...uxfoundation.org> wrote:
> > 
> > > > +#include "intel_rapl.h"
> > > > +#include "../../../fs/sysfs/sysfs.h"  
> > > 
> > > WTF?
> > > 
> > > Oh, that's a sure sign you are not doing something properly, if
> > > you think it's ok to muck around with the internals of sysfs.
> > > 
> > > There's a reason that file is "private", why do you think it's ok
> > > to use it directly?  Did you just think that I somehow "forgot"
> > > to put it in the proper include directory?
> > I did feel unsure about this but i saw some precedence in the
> > kernel.
> 
> Someone else is doing this with the sysfs api?  I don't see any other
> code in Linus's tree doing this at the moment, where did you see this?
> Let me know and I'll fix it up right away.
> 
no, i did not mean sysfs api. I mean include internal header files via
#include ../../ 
e.g.in drivers/usb/image/microtek.c

#include "../../scsi/scsi.h"
#include <scsi/scsi_host.h>

> > Anyway, I needed a way to validate a userspace file passed to rapl
> > driver belong to the same sysfs directory. I will look for
> > alternative ways.
> 
> What do you mean by this?  What exactly are you trying to do?  No
> normal driver code should _ever_ call sysfs functions directly, nor
> should they ever care about sysfs internals.
> 
i did not call sysfs internal calls, just need to use 
struct sysfs_dirent {}

to do the following sanity check against user passed event control file,
it is still not a 100% strong check. 
	/* check if the cfile belongs to the same rapl domain */
	if (strcmp(rd->kobj.sd->s_name,
			cfile->f_dentry->d_parent->d_name.name)) {
		pr_debug("cfile does not belong to domain %s\n",
			rd->kobj.sd->s_name);
		ret = -EINVAL;
		goto exit_cleanup_fds;
	}

> And, odds are, you didn't test your code as a module, right, as any
> internal sysfs function that you could get from this .h file, wouldn't
> be exported for a module to use, unless I missed one somewhere?
> 
I did run the driver as module since i didn't use sysfs internal
functions, just the struct. I may be hitting a corner case here, but
for drivers who need to discover sysfs hierarchy would it be useful to
expose some info in struct sysfs_dirent{}?

> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe
> platform-driver-x86" in the body of a message to
> majordomo@...r.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html

[Jacob Pan]

-- 
Thanks,

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