[libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file

[Hu Tao]

On Fri, Mar 25, 2011 at 10:35:13AM -0400, Laine Stump wrote:
> On 03/25/2011 03:00 AM, Hu Tao wrote:
> >On Thu, Mar 24, 2011 at 03:45:26PM -0600, Eric Blake wrote:
> >>On 03/24/2011 02:46 AM, Hu Tao wrote:
> >>>If two or more disks are mapped to the same image file, operating
> >>>on these disks at the same time may corrupt data stored in the
> >>>image file.
> >>>
> >>>changes:
> >>>
> >>>v2:
> >>>
> >>>- allow it for read-only disks
> >>>- compare source files by inode number
> >>>
> >>>+
> >>>+    if (stat(disk->src,&stat1)) {
> >>>+        if (errno != ENOENT) {
> >>>+            /* Can't stat file, for safety treate it as conflicted */
> >>s/treate/treat/
> >>
> >>Won't this will fail on root-squash NFS from qemu:///system?  (Or does
> >>root-squash meant that root can still stat() but just not open() a file?)
> >I think it won't. man stat says:
> >
> >   No permissions are required on the file itself (to stat it)
> >
> >But needs 'r' bit to open().
> 
> 
> root-squash means that root can only do what user "nobody" could do
> with the filesystem. Although it doesn't need read access on the
> file itself, stat() *will* fail if the current user isn't able to
> read the directory containing the file being stat'ed. So, if user
> nobody doesn't have access to the parent directory (ie if
> permissions are xx0, which is very common), then root will not be
> able to stat the file - it will just return -1.
> 
> To make this work properly for root-squash, you'll need to either 1)
> fork a child that does setuid to the qemu user:group and does the
> stat() (a real pain, since you'll need to pass the results back to
> the parent, and can't reasonably log errors while in the child ==
> not recommended), or 2) use virFileOpenAs() to open the file as the
> qemu user (virFileOpenAs() uses SCM_RIGHTS and a child process to do
> this), then do fstat() of the file to get the info you need, and
> close it.
> 
> Keeping libvirt working properly with save images, disk images, and
> pools on root-squash NFS is a never-ending battle (and can lead to
> an intense hatred of NFS!) Basically any time new code is added that
> needs to access any of these things, operation on root-squash is
> broken. Now that we have virFileOpenAs(), it should be much more
> straightforward to code so that root-squash scenarios keep working.
> 
> If anyone is adding some code that does anything with files on disk,
> they don't have a root-squash NFS setup, and they want some testing
> to make sure they haven't broken it, please point out the patch to
> me, and I'll make the time to apply it locally and try it on my NFS
> setup. (I know I should automatically just see it, but the volume on
> libvir-list has increased *a lot* lately, and I frequently find
> myself several days behind).
> 
> 
> >>Overall, I'm worried that this patch is repeating some of danpb's bigger
> >>efforts to integrate a sanlock disk contention avoidance [1].  If a
> >>resource manager is properly hooked to all disks, then we can prevent
> >>contention between domains (and not limit ourself to just single-domain
> >>contention, as in this patch).  On the other hand, this seems like an
> >>easy enough check to do for a single domain
> 
> 
> My opinion is that it's probably much more likely that someone would
> mistakenly use the same disk in two different domains (due to
> cut-paste of XML) rather than using it twice in the same domain
> (that's much easier to notice since the definitions are close to
> each other in the same file).

Agreed.

> 
> So beyond the fact that this patch can't eliminate all erroneous
> duplicate usage, it's not looking for the category that is most
> likely, and thus it will probably have more an effect of providing a
> false sense of security, rather than of catching any duplicate
> usage. That makes me think this may actually do more harm than good.

Agreed.

> 
> 
> >>whether or not we get the
> >>sanlock code completed (that is, timing wise this looks like it could be
> >>ready prior to 0.9.0 while Dan's work is bigger in scope and probably
> >>missed the feature freeze for this month's release).  So I'm not sure
> >>whether to ack this.
> 
> 
> Assuming this patch (or something similar) was accepted, would
> anybody (aside from those of us using upstream builds directly) ever
> get a release that contained this patch, but didn't contain Dan's?
> If the answer is no, then I think that's another reason to NACK this
> and wait for the Dan's patch.


And thanks to all people involved in this thread for your applies. Now
it's clear that danp's snalock patch is a much better solution, so plz
skip this one.

-- 
Thanks,
Hu Tao