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.8346.1608213468155795078 for ; Thu, 17 Dec 2020 05:57:48 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=KZMdvMUd; 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=1608213467; 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=BIpbSA4nSmGuBuXtzimyIlxGFhuDt/zjWt3is49k95I=; b=KZMdvMUdPQm1SVMR/P5kMa4lYdg0bQmkBgmJ3edMDRDlfLC+6SbrHW/MpCPmEWJ1y/rGzX 12qaJPGhXtmVRHG6SmHYu+WXHOik1jOU0OBjh6npc1dUW6Sh6qLKymCmonBMOork+1jVhA r/Gx/XwkVMSfd0QGEZLKkL2ONFrScFM= 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-442-zh0ZEe0LPnSIAOWqMJAJZw-1; Thu, 17 Dec 2020 08:57:43 -0500 X-MC-Unique: zh0ZEe0LPnSIAOWqMJAJZw-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 26BA3107ACE3; Thu, 17 Dec 2020 13:57:42 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-40.ams2.redhat.com [10.36.115.40]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3C103E730; Thu, 17 Dec 2020 13:57:37 +0000 (UTC) Subject: Re: [Virtio-fs] [edk2 PATCH 06/48] OvmfPkg/VirtioFsDxe: introduce the basic FUSE request/response headers To: "Dr. David Alan Gilbert" Cc: devel@edk2.groups.io, virtio-fs@redhat.com, Jordan Justen , =?UTF-8?Q?Philippe_Mathieu-Daud=c3=a9?= , Ard Biesheuvel References: <20201216211125.19496-1-lersek@redhat.com> <20201216211125.19496-7-lersek@redhat.com> <20201217114950.GC4117@work-vm> From: "Laszlo Ersek" Message-ID: <936ba94a-8226-8649-d9af-b5f6382ebbc2@redhat.com> Date: Thu, 17 Dec 2020 14:57:36 +0100 MIME-Version: 1.0 In-Reply-To: <20201217114950.GC4117@work-vm> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 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: 8bit On 12/17/20 12:49, Dr. David Alan Gilbert wrote: > * Laszlo Ersek (lersek@redhat.com) wrote: >> Introduce the VIRTIO_FS_FUSE_REQUEST and VIRTIO_FS_FUSE_RESPONSE >> structures, which are the common headers for the various FUSE >> request/response structures. >> >> Introduce the VirtioFsFuseNewRequest() helper function for populating >> VIRTIO_FS_FUSE_REQUEST, from parameters and from a VIRTIO_FS-level request >> counter. >> >> Introduce the VirtioFsFuseCheckResponse() helper function for verifying >> most FUSE response types that begin with the VIRTIO_FS_FUSE_RESPONSE >> header. >> >> Cc: Ard Biesheuvel >> Cc: Jordan Justen >> Cc: Philippe Mathieu-Daudé >> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3097 >> Signed-off-by: Laszlo Ersek >> --- >> OvmfPkg/Include/IndustryStandard/VirtioFs.h | 49 +++++ >> OvmfPkg/VirtioFsDxe/VirtioFsDxe.h | 17 ++ >> OvmfPkg/VirtioFsDxe/DriverBinding.c | 5 + >> OvmfPkg/VirtioFsDxe/Helpers.c | 216 ++++++++++++++++++++ >> 4 files changed, 287 insertions(+) >> >> diff --git a/OvmfPkg/Include/IndustryStandard/VirtioFs.h b/OvmfPkg/Include/IndustryStandard/VirtioFs.h >> index ea7d80d15d0b..521288b03f1c 100644 >> --- a/OvmfPkg/Include/IndustryStandard/VirtioFs.h >> +++ b/OvmfPkg/Include/IndustryStandard/VirtioFs.h >> @@ -44,9 +44,58 @@ typedef struct { >> // >> // The total number of request virtqueues exposed by the device (i.e., >> // excluding the "hiprio" queue). >> // >> UINT32 NumReqQueues; >> } VIRTIO_FS_CONFIG; >> #pragma pack () >> >> +// >> +// FUSE-related definitions follow. >> +// >> +// From virtio-v1.1-cs01-87fa6b5d8155, 5.11 File System Device: "[...] The >> +// driver acts as the FUSE client mounting the file system. The virtio file >> +// system device provides the mechanism for transporting FUSE requests [...]" >> +// >> +// Unfortunately, the documentation of the FUSE wire protocol is lacking. The >> +// Virtio spec (as of this writing) simply defers to >> +// "include/uapi/linux/fuse.h" in the Linux kernel source -- see the reference >> +// in virtio spec file "introduction.tex", at commit 87fa6b5d8155. >> +// >> +// Of course, "include/uapi/linux/fuse.h" is a moving target (the virtio spec >> +// does not specify a particular FUSE interface version). The OvmfPkg code >> +// targets version 7.31, because that's the lowest version that the QEMU >> +// virtio-fs daemon supports at this time -- see QEMU commit 72c42e2d6551 >> +// ("virtiofsd: Trim out compatibility code", 2020-01-23). > > Fuse has it's own in built protocol versioning and negotiation, and as I > remember it's the version of the header in the Linux kernel that's the > definitive version. I think it would have also been safe just to take > the latest released kernel version 7.32 (kernel commit c6ff213fe5b8) introduces "submount support", which does not seem relevant. The strategy we follow in "OvmfPkg/Include/IndustryStandard" is: never include or convert header files from other projects whole-sale; always evaluate every structure and macro one by one for necessity, and if a definition is really needed for the driver being implemented, add that definition manually, and natively to the edk2 coding style. This approach has caused zero friction or extra maintenance work over the years, and it allows for a native look & feel. (It's a different question what people think of edk2's "native" look and feel :)) A counter-example is the Xen headers in "OvmfPkg/Include/IndustryStandard/Xen", which were mass-imported and then fixed up separately for edk2 compatibility at some point -- much to my dismay. They feel totally foreign in edk2 and they've never stopped being a thorn in my side. It's not just the headers of course: when you *use* those macros and typedefs in the actual driver code, it shows. So, as long as it's up to me, OVMF never "just takes" but "adds from zero", and I do stick with the absolute minimum needed. Thanks, Laszlo > > Dave > >> +// Correspondingly, Linux's "include/uapi/linux/fuse.h" is consulted as checked >> +// out at commit (c6ff213fe5b8^) = d78092e4937d ("fuse: fix page dereference >> +// after free", 2020-09-18); that is, right before commit c6ff213fe5b8 ("fuse: >> +// add submount support to ", 2020-09-18) introduces FUSE >> +// interface version 7.32. >> +// >> +#define VIRTIO_FS_FUSE_MAJOR 7 >> +#define VIRTIO_FS_FUSE_MINOR 31 >> + >> +#pragma pack (1) >> +// >> +// Request-response headers common to all request types. >> +// >> +typedef struct { >> + UINT32 Len; >> + UINT32 Opcode; >> + UINT64 Unique; >> + UINT64 NodeId; >> + UINT32 Uid; >> + UINT32 Gid; >> + UINT32 Pid; >> + UINT32 Padding; >> +} VIRTIO_FS_FUSE_REQUEST; >> + >> +typedef struct { >> + UINT32 Len; >> + INT32 Error; >> + UINT64 Unique; >> +} VIRTIO_FS_FUSE_RESPONSE; >> +#pragma pack () >> + >> #endif // VIRTIO_FS_H_ >> diff --git a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h >> index 12acbd6dc359..f7eae9a4b71a 100644 >> --- a/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h >> +++ b/OvmfPkg/VirtioFsDxe/VirtioFsDxe.h >> @@ -39,16 +39,17 @@ typedef struct { >> // field init function init depth >> // ----------- ------------------ ---------- >> UINT64 Signature; // DriverBindingStart 0 >> VIRTIO_DEVICE_PROTOCOL *Virtio; // DriverBindingStart 0 >> VIRTIO_FS_LABEL Label; // VirtioFsInit 1 >> UINT16 QueueSize; // VirtioFsInit 1 >> VRING Ring; // VirtioRingInit 2 >> VOID *RingMap; // VirtioRingMap 2 >> + UINT64 RequestId; // DriverBindingStart 0 >> EFI_EVENT ExitBoot; // DriverBindingStart 0 >> EFI_SIMPLE_FILE_SYSTEM_PROTOCOL SimpleFs; // DriverBindingStart 0 >> } VIRTIO_FS; >> >> #define VIRTIO_FS_FROM_SIMPLE_FS(SimpleFsReference) \ >> CR (SimpleFsReference, VIRTIO_FS, SimpleFs, VIRTIO_FS_SIG); >> >> // >> @@ -127,16 +128,32 @@ VirtioFsSgListsValidate ( >> >> EFI_STATUS >> VirtioFsSgListsSubmit ( >> IN OUT VIRTIO_FS *VirtioFs, >> IN OUT VIRTIO_FS_SCATTER_GATHER_LIST *RequestSgList, >> IN OUT VIRTIO_FS_SCATTER_GATHER_LIST *ResponseSgList OPTIONAL >> ); >> >> +EFI_STATUS >> +VirtioFsFuseNewRequest ( >> + IN OUT VIRTIO_FS *VirtioFs, >> + OUT VIRTIO_FS_FUSE_REQUEST *Request, >> + IN UINT32 RequestSize, >> + IN UINT32 Opcode, >> + IN UINT64 NodeId >> + ); >> + >> +EFI_STATUS >> +VirtioFsFuseCheckResponse ( >> + IN VIRTIO_FS_SCATTER_GATHER_LIST *ResponseSgList, >> + IN UINT64 RequestId, >> + OUT UINTN *TailBufferFill >> + ); >> + >> // >> // EFI_SIMPLE_FILE_SYSTEM_PROTOCOL member functions for the Virtio Filesystem >> // driver. >> // >> >> EFI_STATUS >> EFIAPI >> VirtioFsOpenVolume ( >> diff --git a/OvmfPkg/VirtioFsDxe/DriverBinding.c b/OvmfPkg/VirtioFsDxe/DriverBinding.c >> index b888158a805d..4a2787a50a6e 100644 >> --- a/OvmfPkg/VirtioFsDxe/DriverBinding.c >> +++ b/OvmfPkg/VirtioFsDxe/DriverBinding.c >> @@ -79,16 +79,21 @@ VirtioFsBindingStart ( >> goto FreeVirtioFs; >> } >> >> Status = VirtioFsInit (VirtioFs); >> if (EFI_ERROR (Status)) { >> goto CloseVirtio; >> } >> >> + // >> + // Initialize the FUSE request counter. >> + // >> + VirtioFs->RequestId = 1; >> + >> Status = gBS->CreateEvent (EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK, >> VirtioFsExitBoot, VirtioFs, &VirtioFs->ExitBoot); >> if (EFI_ERROR (Status)) { >> goto UninitVirtioFs; >> } >> >> VirtioFs->SimpleFs.Revision = EFI_SIMPLE_FILE_SYSTEM_PROTOCOL_REVISION; >> VirtioFs->SimpleFs.OpenVolume = VirtioFsOpenVolume; >> diff --git a/OvmfPkg/VirtioFsDxe/Helpers.c b/OvmfPkg/VirtioFsDxe/Helpers.c >> index 88264d4b264c..5bd2dc641f6d 100644 >> --- a/OvmfPkg/VirtioFsDxe/Helpers.c >> +++ b/OvmfPkg/VirtioFsDxe/Helpers.c >> @@ -693,8 +693,224 @@ VirtioFsSgListsSubmit ( >> if (!EFI_ERROR (Status) && EFI_ERROR (UnmapStatus)) { >> Status = UnmapStatus; >> } >> } >> } >> >> return Status; >> } >> + >> +/** >> + Set up the fields of a new VIRTIO_FS_FUSE_REQUEST object. >> + >> + The function may only be called after VirtioFsInit() returns successfully and >> + before VirtioFsUninit() is called. >> + >> + @param[in,out] VirtioFs The Virtio Filesystem device that the request is >> + being prepared for. The "VirtioFs->RequestId" field >> + will be copied into "Request->Unique". On output (on >> + successful return), "VirtioFs->RequestId" will be >> + incremented. >> + >> + @param[out] Request The VIRTIO_FS_FUSE_REQUEST object whose fields are to >> + be set. >> + >> + @param[in] RequestSize The total size of the request, including >> + sizeof(VIRTIO_FS_FUSE_REQUEST). >> + >> + @param[in] Opcode The VIRTIO_FS_FUSE_OPCODE that identifies the command >> + to send. >> + >> + @param[in] NodeId The inode number of the file that the request refers >> + to. >> + >> + @retval EFI_INVALID_PARAMETER RequestSize is smaller than >> + sizeof(VIRTIO_FS_FUSE_REQUEST). >> + >> + @retval EFI_OUT_OF_RESOURCES "VirtioFs->RequestId" is MAX_UINT64, and can >> + be incremented no more. >> + >> + @retval EFI_SUCCESS Request has been populated, >> + "VirtioFs->RequestId" has been incremented. >> +**/ >> +EFI_STATUS >> +VirtioFsFuseNewRequest ( >> + IN OUT VIRTIO_FS *VirtioFs, >> + OUT VIRTIO_FS_FUSE_REQUEST *Request, >> + IN UINT32 RequestSize, >> + IN UINT32 Opcode, >> + IN UINT64 NodeId >> + ) >> +{ >> + if (RequestSize < sizeof *Request) { >> + return EFI_INVALID_PARAMETER; >> + } >> + >> + if (VirtioFs->RequestId == MAX_UINT64) { >> + return EFI_OUT_OF_RESOURCES; >> + } >> + >> + Request->Len = RequestSize; >> + Request->Opcode = Opcode; >> + Request->Unique = VirtioFs->RequestId++; >> + Request->NodeId = NodeId; >> + Request->Uid = 0; >> + Request->Gid = 0; >> + Request->Pid = 1; >> + Request->Padding = 0; >> + >> + return EFI_SUCCESS; >> +} >> + >> +/** >> + Check the common FUSE response format. >> + >> + The first buffer in the response scatter-gather list is assumed a >> + VIRTIO_FS_FUSE_RESPONSE structure. Subsequent response buffers, if any, up to >> + and excluding the last one, are assumed fixed size. The last response buffer >> + may or may not be fixed size, as specified by the caller. >> + >> + This function may only be called after VirtioFsSgListsSubmit() returns >> + successfully. >> + >> + @param[in] ResponseSgList The scatter-gather list that describes the >> + response part of the exchange -- the buffers that >> + the Virtio Filesystem device filled in during the >> + virtio transfer. >> + >> + @param[in] RequestId The request identifier to which the response is >> + expected to belong. >> + >> + @param[out] TailBufferFill If NULL, then the last buffer in ResponseSgList >> + is considered fixed size. Otherwise, the last >> + buffer is considered variable size, and on >> + successful return, TailBufferFill reports the >> + number of bytes in the last buffer. >> + >> + @retval EFI_INVALID_PARAMETER TailBufferFill is not NULL (i.e., the last >> + buffer is considered variable size), and >> + ResponseSgList->NumVec is 1. >> + >> + @retval EFI_INVALID_PARAMETER The allocated size of the first buffer does >> + not match sizeof(VIRTIO_FS_FUSE_RESPONSE). >> + >> + @retval EFI_PROTOCOL_ERROR The VIRTIO_FS_FUSE_RESPONSE structure in the >> + first buffer has not been fully populated. >> + >> + @retval EFI_PROTOCOL_ERROR "VIRTIO_FS_FUSE_RESPONSE.Len" in the first >> + buffer does not equal the sum of the >> + individual buffer sizes (as populated). >> + >> + @retval EFI_PROTOCOL_ERROR "VIRTIO_FS_FUSE_RESPONSE.Unique" in the first >> + buffer does not equal RequestId. >> + >> + @retval EFI_PROTOCOL_ERROR "VIRTIO_FS_FUSE_RESPONSE.Error" in the first >> + buffer is zero, but a subsequent fixed size >> + buffer has not been fully populated. >> + >> + @retval EFI_DEVICE_ERROR "VIRTIO_FS_FUSE_RESPONSE.Error" in the first >> + buffer is nonzero. The caller may investigate >> + "VIRTIO_FS_FUSE_RESPONSE.Error". Note that the >> + completeness of the subsequent fixed size >> + buffers is not verified in this case. >> + >> + @retval EFI_SUCCESS Verification successful. >> +**/ >> +EFI_STATUS >> +VirtioFsFuseCheckResponse ( >> + IN VIRTIO_FS_SCATTER_GATHER_LIST *ResponseSgList, >> + IN UINT64 RequestId, >> + OUT UINTN *TailBufferFill >> + ) >> +{ >> + UINTN NumFixedSizeVec; >> + VIRTIO_FS_FUSE_RESPONSE *CommonResp; >> + UINT32 TotalTransferred; >> + UINTN Idx; >> + >> + // >> + // Ensured by VirtioFsSgListsValidate(). >> + // >> + ASSERT (ResponseSgList->NumVec > 0); >> + >> + if (TailBufferFill == NULL) { >> + // >> + // All buffers are considered fixed size. >> + // >> + NumFixedSizeVec = ResponseSgList->NumVec; >> + } else { >> + // >> + // If the last buffer is variable size, then we need that buffer to be >> + // different from the first buffer, which is considered a >> + // VIRTIO_FS_FUSE_RESPONSE (fixed size) structure. >> + // >> + if (ResponseSgList->NumVec == 1) { >> + return EFI_INVALID_PARAMETER; >> + } >> + NumFixedSizeVec = ResponseSgList->NumVec - 1; >> + } >> + >> + // >> + // The first buffer is supposed to carry a (fully populated) >> + // VIRTIO_FS_FUSE_RESPONSE structure. >> + // >> + if (ResponseSgList->IoVec[0].Size != sizeof *CommonResp) { >> + return EFI_INVALID_PARAMETER; >> + } >> + if (ResponseSgList->IoVec[0].Transferred != ResponseSgList->IoVec[0].Size) { >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + // >> + // FUSE must report the same number of bytes, written by the Virtio >> + // Filesystem device, as the virtio transport does. >> + // >> + CommonResp = ResponseSgList->IoVec[0].Buffer; >> + TotalTransferred = 0; >> + for (Idx = 0; Idx < ResponseSgList->NumVec; Idx++) { >> + // >> + // Integer overflow and truncation are not possible, based on >> + // VirtioFsSgListsValidate() and VirtioFsSgListsSubmit(). >> + // >> + TotalTransferred += (UINT32)ResponseSgList->IoVec[Idx].Transferred; >> + } >> + if (CommonResp->Len != TotalTransferred) { >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + // >> + // Enforce that FUSE match our request ID in the response. >> + // >> + if (CommonResp->Unique != RequestId) { >> + return EFI_PROTOCOL_ERROR; >> + } >> + >> + // >> + // If there is an explicit error report, skip checking the transfer >> + // counts for the rest of the fixed size buffers. >> + // >> + if (CommonResp->Error != 0) { >> + return EFI_DEVICE_ERROR; >> + } >> + >> + // >> + // There was no error reported, so we require that the Virtio Filesystem >> + // device populate all fixed size buffers. We checked this for the very first >> + // buffer above; let's check the rest (if any). >> + // >> + ASSERT (NumFixedSizeVec >= 1); >> + for (Idx = 1; Idx < NumFixedSizeVec; Idx++) { >> + if (ResponseSgList->IoVec[Idx].Transferred != >> + ResponseSgList->IoVec[Idx].Size) { >> + return EFI_PROTOCOL_ERROR; >> + } >> + } >> + >> + // >> + // If the last buffer is considered variable size, report its filled size. >> + // >> + if (TailBufferFill != NULL) { >> + *TailBufferFill = ResponseSgList->IoVec[NumFixedSizeVec].Transferred; >> + } >> + >> + return EFI_SUCCESS; >> +} >> -- >> 2.19.1.3.g30247aa5d201 >> >> >> >> _______________________________________________ >> Virtio-fs mailing list >> Virtio-fs@redhat.com >> https://www.redhat.com/mailman/listinfo/virtio-fs