From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.16351.1608418497150891981 for ; Sat, 19 Dec 2020 14:54:57 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JKNCxZgP; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1608418496; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=EjFPCsaP8GG9b9XKdwYQuxDrIjJ8IwZ1lxtLjOUGJXo=; b=JKNCxZgPWemEhIs+hA+YhD1Vo3OGek/8q4zBAyM5mcGrGFJCrSBs/bp79fDbfq5OYbavcG QJ2sYgbPjcB0Vhb0nXalsyiBbXU6giMa3KFaKu5gzOev5VTsJCVeCmSSUkeV8k3230xMgY pDl8nLE4IBdRy8VwAlLNtNU4bd7l6IE= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-524-2ZrTtTUJNNm3EvYRsoqggg-1; Sat, 19 Dec 2020 17:54:54 -0500 X-MC-Unique: 2ZrTtTUJNNm3EvYRsoqggg-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 191F5801817; Sat, 19 Dec 2020 22:54:53 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-53.ams2.redhat.com [10.36.112.53]) by smtp.corp.redhat.com (Postfix) with ESMTP id 22E896E528; Sat, 19 Dec 2020 22:54:47 +0000 (UTC) Subject: Re: [edk2 PATCH 42/48] OvmfPkg/VirtioFsDxe: add helper for composing rename/move destination path From: "Laszlo Ersek" To: Ard Biesheuvel , devel@edk2.groups.io, virtio-fs@redhat.com Cc: Jordan Justen , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= References: <20201216211125.19496-1-lersek@redhat.com> <20201216211125.19496-43-lersek@redhat.com> Message-ID: <7083dc33-ae05-105c-5784-de24c1818cdf@redhat.com> Date: Sat, 19 Dec 2020 23:54:47 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 12/19/20 23:40, Laszlo Ersek wrote: > On 12/18/20 18:39, Ard Biesheuvel wrote: >> On 12/16/20 10:11 PM, Laszlo Ersek wrote: >>> The EFI_FILE_PROTOCOL.SetInfo() member is somewhat under-specified; one of >>> its modes of operation is renaming/moving the file. >>> >>> In order to create the destination pathname in canonical format, 2*2=4 >>> cases have to be considered. For the sake of discussion, assume the >>> current canonical pathname of a VIRTIO_FS_FILE is "/home/user/f1.txt". >>> Then, consider the following rename/move requests from >>> EFI_FILE_PROTOCOL.SetInfo(): >>> >>> Destination requested Destination Move into Destination in >>> by SetInfo() relative? directory? canonical format >>> --------------------- ----------- ---------- ----------------------- >>> L"\\dir\\f2.txt" no no "/dir/f2.txt" >> >> What happens in the above case if /dir/f2.txt is an existing directory? > > > Short answer: > > The present patch only constructs the destination pathname. The rename > attempt you describe is caught > > - either by the subsequent patch, if the existing dest directory is open > by the guest driver, > > - or else, by the host kernel, due to the RENAME_NOREPLACE flag set in > the patch before this one. ... I'm sorry, I need to correct a bit: the guest-side check I quoted below is not meant to protect from the destination being overwritten, it is supposed to protect against the *source disappearing* (and against breaking other VIRTIO_FS_FILEs' canonical pathnames due to that). So, the answer to your question is: it is caught by RENAME_NOREPLACE. Everything else I said stands, it's just that the particular guest-side check is related to a different question -- namely, "what if '/home/user/f1.txt' is a directory". Thanks, and sorry about the confusion, Laszlo > Long (very long) answer, in opposite order of the above cases: > > - If "/dir/f2.txt" were an existing name (regardless of the type of the > host-side inode that it referred to), then the FUSE_RENAME2 request > would fail: the host kernel would reject the renameat2() system call > made by virtiofsd. This would be due to the RENAME_NOREPLACE flag: > > https://man7.org/linux/man-pages/man2/rename.2.html > include/uapi/linux/fs.h > > which is set in > > [edk2 PATCH 41/48] > OvmfPkg/VirtioFsDxe: implement the wrapper function for FUSE_RENAME2 > > using the macro name VIRTIO_FS_FUSE_RENAME2_REQ_F_NOREPLACE. > > Thus, if the request reached the host kernel, an -EEXIST errno would > come back to the guest driver. > > ( > > - If the movement source were a non-directory and the destination were a > directory, then that would fail (also in the host kernel at the > latest) even with the simpler (flag-less) FUSE_RENAME request, which > virtiofsd translates to the renameat() syscall, with -EISDIR. > > - If both source and destination were directories, and the destination > were not empty, then even the flag-less renameat() would fail with > -ENOTEMPTY. > > - If both source and destination were directories, and the destination > were empty, then renameat() would replace the destination [*]; but > renameat2() with RENAME_NOREPLACE will not. > > [*] mkdir source-dir target-dir > ls -lid source-dir target-dir > touch source-dir/file.txt > mv -T source-dir target-dir > ls -lid target-dir > ls -l target-dir/file.txt > > ) > > > Then, in addition to RENAME_NOREPLACE, there's a guest-side check in > > [edk2 PATCH 43/48] > OvmfPkg/VirtioFsDxe: handle file rename/move in EFI_FILE_PROTOCOL.SetInfo > > that was inspired by "FatPkg/EnhancedFatDxe". Namely: > >> + // >> + // Check if the rename would break the canonical pathnames of other >> + // VIRTIO_FS_FILE instances of the same VIRTIO_FS. >> + // >> + if (VirtioFsFile->IsDirectory) { >> + UINTN PathLen; >> + LIST_ENTRY *OpenFilesEntry; >> + >> + PathLen = AsciiStrLen (VirtioFsFile->CanonicalPathname); >> + BASE_LIST_FOR_EACH (OpenFilesEntry, &VirtioFs->OpenFiles) { >> + VIRTIO_FS_FILE *OtherFile; >> + >> + OtherFile = VIRTIO_FS_FILE_FROM_OPEN_FILES_ENTRY (OpenFilesEntry); >> + if (OtherFile != VirtioFsFile && >> + AsciiStrnCmp (VirtioFsFile->CanonicalPathname, >> + OtherFile->CanonicalPathname, PathLen) == 0 && >> + (OtherFile->CanonicalPathname[PathLen] == '\0' || >> + OtherFile->CanonicalPathname[PathLen] == '/')) { >> + // >> + // OtherFile refers to the same directory as VirtioFsFile, or is a >> + // (possibly indirect) child of the directory referred to by >> + // VirtioFsFile. >> + // >> + Status = EFI_ACCESS_DENIED; >> + goto FreeDestination; >> + } >> + } >> + } > > This is why "VIRTIO_FS.OpenFiles" is a list, and not just a reference > count -- for this simple guest-side check, I needed to go through the > other open VIRTIO_FS_FILEs for the same VIRTIO_FS one by one. Just > knowing how many of them existed wouldn't be enough. > > This guest-side check is by no means foolproof; after all, you can do > whatever you want on the host side, underneath the guest driver's feet. > > But, catching such "async tricks" is not a goal for this driver. > EFI_FILE_PROTOCOL is not equipped to deal with such async changes by > design. At best, it can return EFI_MEDIA_CHANGED, when (e.g.) removable > media is replaced. But even if I could detect such a situation in the > virtio-fs driver, it would be counter-productive: when a host-side file > changes, that's something the guest wants to pick up one way or another, > we don't want the driver to switch to returning EFI_MEDIA_CHANGED for > all further requests indiscriminately. > > Synchronization between host and guest is pretty simple for the > interactive use case: whenever your shell prompt returns, on the host or > in the guest (meaning the UEFI shell prompt in the latter), you can > Alt-TAB to the other terminal, and manipulate files from there. > > Synchronization between host-side and guest-side scripts should be > possible with polling directory listings, and renaming / moving regular > files (files should be prepared in "private" directories, and then moved > into place when done, for the other side to notice). > > So, the above guest-side check exists for the usual case when the > relevant sub-hierarchy of the shared directory doesn't change > asynchronously to VIRTIO_FS_FILE's being open; when the guest-side check > fires in this optimistic situation, the FUSE_RENAME2 request isn't even > sent. And for when the guest-side check isn't good enough, that's when > the RENAME_NOREPLACE flag becomes relevant -- I wanted to avoid > unwittingly deleting entries in the shared directory by guest-initiated > renames. > > Sorry about the long answer. I feel it's not really possible to address > your question without talking about asynchrony between host and guest. I > had thought about it before, and I figured, shoehorning a shared > filesystem into a non-shared filesystem abstraction should be acceptable > this way. The use case is to support Alt-TABbing between your guest and > host terminals, and running such UEFI unit tests in the guest that take > input from the host filesystem and produce output to the host > filesystem. A highly async / parallel operation is a non-goal. > > Thanks! > Laszlo