Re: writing config to filesystem (was: Re: [PATCHv2 0/12] target_core_pr.c cleanups)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 2013-05-20 at 11:39 -0700, Andy Grover wrote:
> On 05/17/2013 06:35 PM, Nicholas A. Bellinger wrote:
> > No, it's the pulling out of aptpl metadata from configfs at random times
> > that can lead to incorrect state.  Just having that bit exposed in patch
> > #12 doesn't make any sense given the NAK on the original #13, which is
> > why I'm not applying it either.
> 
> Why would reading aptpl metadata from configfs lead to incorrect state?
> 

Again, look closer at the code.  ALL_TG_PT is one example where someone
randomly reading aptpl metadata from configfs can get the incorrect
state, but there are certainly more cases than that.

There's a reason why aptpl updates are currently performed at specific
times in the PR logic path, and it's an incorrect assumption that they
can be read at any time from user-space and get the correct state for
all registrations and reservations.

> > Huh..?  kernel_write translates to vfs_write..  So your saying that
> > these are going away long-term..?
> 
> My example was crappy. vfs_write has its uses -- I have no problem with 
> fileio backstore using it -- but basically the kernel should not (and 
> generally does not) write configuration info to the filesystem; config 
> is pushed down into the kernel by userspace, and read by userspace via 
> mechanisms besides the kernel writing files.
> 
> >> Policy should not go in the kernel.
> >
> > Nonsense, this has nothing to do with policy.
> 
> Here's an excerpt from an article GregKH (CCd) wrote:
> 
> (talks about reading config from fs but also applies to writing)
> 
> http://www.linuxjournal.com/article/8110
> 
> "Trying to protect the kernel from dumb programming errors is not the 
> most important reason for not allowing drivers to read files. The 
> biggest issue is policy. Linux kernel programmers try to flee from the 
> word policy as fast as they can. They almost never want to force the 
> kernel to force a policy on to user space that can possibly be avoided. 
> Having a module read a file from a filesystem at a specific location 
> forces the policy of the location of that file to be set. If a Linux 
> distributor decides the easiest way to handle all configuration files 
> for the system is to place them in the /var/black/hole/of/configs, this 
> kernel module has to be modified to support this change. This is 
> unacceptable to the Linux kernel community."
> 

The only policy is the the hardcoded PR aptpl metadata path, and like I
said before, go ahead and submit a patch to allow the changing of the
path.

Aside from that, and given PR aptpl metadata is not being *read* by
kernel-space, your argument for an upcall and user-space dependency in
an existing I/O path is extremely thin.

>
> > Whatever you want to do can be done with the existing code using inotify
> > on the metadata files in question, with the exact same formatting as the
> > changes you've already proposed.
> 
> Yuck. Run a daemon??
> 
> We're using configfs for everything else, so we just need to make this 
> also available there, and then poke userspace when needed to save it and 
> guard against unexpected reboots. call_usermodehelper() seems like the 
> right approach.
> 

The more I think about this, the less and less sense it makes any sense
to me.

Why you think that exporting possibly dozens of key=value formatted PR
registrations via a single configfs attribute is OK is beyond me, but it
certainly breaks every rule about keeping a single value for sysfs /
configfs attribute output that I've heard about.

--nab

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Linux SCSI]     [Kernel Newbies]     [Linux SCSI Target Infrastructure]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Device Mapper]

  Powered by Linux