On Tue, Mar 16, 2010 at 11:01:10AM -0400, Dave Allan wrote: > On 03/16/2010 06:22 AM, Daniel P. Berrange wrote: > >On Mon, Mar 15, 2010 at 05:21:26PM -0400, Dave Allan wrote: > >> > >>Now that 0.7.7 is done, we need to revisit fs pool building. > >> > >>Because it is impossible to implement a check for arbitrary existing > >>data, the only safe option is to require the overwrite flag in all > >>cases. If we do not require the flag in all cases, virt-manager and > >>virsh will format unknown partitions without providing any indication to > >>the user that a destructive operation is about to take place. The only > >>input from the user will be the selection of the partition. All other > >>instances of destructive behavior require explicit confirmation from the > >>user, or as Cole aptly put it, are loudly warned about by virt-manager. > >> I wish that a safe alternative existed, but none does. > > > >There are two alternatives that are safe > > Unfortunately, no. There is no programatic way to detect the presence > of arbitrary data on a partition. Any attempt to do so is false > security. We can, as you point out, determine in some cases that the > user *is* going to overwrite something, but we cannot determine in all > cases that the user *is not* going to overwrite something. > > > 1. Do a per filesystem magic check on the volume in question. libvirt has > > a list of filesystems that I knows about "ext2", "ext3", "ext4", > > "ufs", "iso9660", "udf", "gfs", "gfs2", "vfat", "hfs+", "xfs", "ocfs2" > > All of these will have an easily identified magic header that could > > be positively checked for. > > > >Or > > > > 2. Do a check for the first 512 or 4096 bytes being all zeros. This > > should > > detect the absence of any filesystem AFAIK. > > And that's the problem. We can detect filesystems *that we know of*. > We do not and cannot know the universe of partition formats that exist. > Again, if we pretend we do all we're going to do is give users a false > sense of security when the only real security is asking the user "You're > about to destroy data, are you certain you have the right partition?" I don't think we need to care about all possible types of data in the world. We already have exactly the issue you describe for the logical and disk storage pools. We blindly run 'pvcreate' on all disk paths you pass in the logical pool XML. pvcreate will check if it is alrady a LVM physical volume. It will happily overwrite any other type of data on that devices. For the disk pool we just run 'parted mklabel' on the path, again not checking for any random data that might be there. We don't need to protect against all possible data that might be on the disk. Primarily we should be protecting against the execution of virStoragePoolBuild() twice in a row. This implies the safety checks that I outline above. This also protects against the most likely common sources of data the user might have there already. I agree with you that this isn't 100% protection from all possible risks. Adding a OVERWRITE flag on its own is not actually reducing the risk of the API though. It is merely moving the risk from one part of the API to another. The overall risk remains the same, except it avoids one specific virt-manager bug. Adding checking to the Build() API with flags=0 does reduce the real risk in all scenarios that libvirt itself directly supports. > > >The semantics we should have for these APIs are > > > > - virStoragePoolBuild(flags=0) - format the > > filesystem/volgroup/partition table > > only if not already formatted. > > - virStoragePoolBuild(flags=OVERWRITE) - unconditionally format > > - virStoragePoolDelete() - wipe data such that Build(flags=0) is > > guaranteed > > to be successful next time. > > > >So I see several things that need to be implemented > > > > - Make disk& logical pool backends check for existing formatted data > > - Implement the 'Delete' operation for all pool types, > > - Add (checked) formatting of filesystem pools > > - Add OVERWRITE flag to disk, logical, filesystem pool types to skip > > the check > > > >>The attached patch implements this behavior. > > > >NACK in this form. > > We clearly disagree, so I think we need some additional voices to weigh > in here. The most fundemental requirement here is that we provide API semantics that are consistent across all pool types. Second, we need to reduce the risk of this API bth in general, and to protect against the virt-manager bug. This patch makes the FS pool even more of a special case, and does not do anything to address the identical risk scenario *already* present in LVM & disk pool types impl of the virStoragePoolBuild(). Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://deltacloud.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list