linux-kernel - Re: [ckrm-tech] [PATCH 0/5] Allow more than PAGESIZE data read in configfs

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: <1160709776.18766.451.camel@localhost.localdomain>
Date:	Thu, 12 Oct 2006 20:22:56 -0700
From:	Matt Helsley <matthltc@...ibm.com>
To:	Greg KH <greg@...ah.com>
Cc:	Joel Becker <Joel.Becker@...cle.com>,
	Paul Menage <menage@...gle.com>, linux-kernel@...r.kernel.org,
	Chandra Seetharaman <sekharan@...ibm.com>,
	ckrm-tech@...ts.sourceforge.net,
	Shailabh Nagar <nagar@...son.ibm.com>
Subject: Re: [ckrm-tech] [PATCH 0/5] Allow more than PAGESIZE data read in
	configfs

On Thu, 2006-10-12 at 16:54 -0700, Greg KH wrote:
> On Wed, Oct 11, 2006 at 07:17:44PM -0700, Matt Helsley wrote:
> > On Wed, 2006-10-11 at 15:39 -0700, Greg KH wrote:
> > > On Tue, Oct 10, 2006 at 06:28:51PM -0700, Joel Becker wrote:
> > > > On Tue, Oct 10, 2006 at 05:49:59PM -0700, Matt Helsley wrote:
> > > > > 	We want to be able to export a sequence of small (<< 1 page),
> > > > > homogenous, unstructured (scalar), attributes through configfs using the
> > > > > same file. While this is rather specific, I'd guess it would be a common
> > > > > occurrence.
> > > > 
> > > > 	Pray tell, why?  "One attribute per file" is the mantra here.
> > > > You really should think hard before you break it.  Simple heuristic:
> > > > would you have to parse the buffer?  Then it's wrong.
> > > 
> > > I agree.  You are trying to use configfs for something that it is not
> > > entended to be used for.  If you want to write/read large numbers of
> > 
> > 	I disagree with your assertion that we're abusing configfs. "one value
> > per file" is not the purpose of configfs.
> > 
> > 	The purpose of configfs is to allow userspace to create and manipulate
> > kernel objects whose lifetime is under the control of userspace. That
> > perfectly matches the idea of being able to create, manipulate, and
> > destroy a resource group from userspace.
> > 
> > 	"one value per file" is a phrase describing what configfs and sysfs
> > files should normally look like. However it's not a rule since there is
> > precedent for sysfs files that require parsing:
> > /sys/devices/pciXXXX:XX/XXXX:XX:XX.X/resource
> 
> Hold over from old procfs use of pci resources.  And now a requirement
> that X uses this, after it was pointed out to me.
>
> > /sys/block/hda/stat
> 
> I never have liked that, it belongs somewhere else.  Please move it.
>
> > /sys/block/hda/dev
> 
> That is merely a dev_t, a single value.

It's a single value internally but it's presented as multiple values --
and it needs to be parsed. Following the guidelines it should have been
a directory with a single value per file:

/sys/block/hda/dev/major
/sys/block/hda/dev/minor

So the parsing test cited earlier fails. It's not a matter of whether
parsing is required but of how complex the parsing is.

> Come on, there are way worse violators[1] of the "one value per file"

You mean like /sys/class/input/input0/modalias ?

It was not my intention to catalog all of the violators -- only to point
out that there are counterexamples that prove it's not a rule as you've
asserted.

> rule in sysfs that you could have found if you wanted to try to declare
> how much it is not being followed in places.  But if you look, the ratio
> of good vs. bad usage is still very high, and keeping it high is my
> goal.
>
> > > What happened to your old ckrmfs?  I thought you were handling all of
> > > this in that.
> > 
> > 	We dropped RCFS more than 1 year ago after feedback suggested we should
> > try to share as much kernel code as possible. Other than the 1 page
> > limit to the list of pids, configfs is a perfect match for what we need.
> 
> Feedback from whom?  I know I sure liked the fact that you did your own
> filesystem, despite the bugs that were found in it at times :)

	This article on LWN suggested the idea of using configfs:
http://lwn.net/Articles/148180/
Though I think the idea may have come up earlier I haven't been able to
find it with Google. Shailabh may be able to pinpoint the exact source
of the idea.

	We also got feedback along the lines of "it's too
big" ( http://lwn.net/Articles/145135/ ) which motivated us to find ways
to shrink it. Using appropriate kernel systems that already exist is an
excellent way to reduce the size of the patches and cut down on the bugs
-- and we can contribute by fixing some configfs bugs. Of course using
configfs saves quite a bit of code:
http://lwn.net/Articles/149275/

	I've also gotten the impression that new filesystem code tends to
distract (or, worse, confuse) folks from the meat of the patches. It
takes up review time that's better spent on the resource groups core and
controllers.

Cheers,
	-Matt Helsley

-
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/