[PATCH resend v5 4/5] mountmgr: Add support for volume information queries.
This patch allows the mountmgr to handle the volume information request IRP, allowing volume information to be queried from the mountmgr by ntdll. v5: No change v4: Reordered (no content change) v3: Use SystemBuffer instead of UserBuffer (see 8e98dcd42e13bfeb5a2397ff9bf1c7f63c224e23) v2: Per Zeb's request, now storing the volume with the disk_device so that the lookup doesn't require a search. Best, Erich
February 4, 2021 10:40 AM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
@@ -1826,6 +1828,117 @@ static void query_property( struct disk_device *device, IRP *irp ) [...] +done: + LeaveCriticalSection( &device_section ); + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return STATUS_SUCCESS; +}
Shouldn't that return what's in io->u.Status? I thought MS recommended doing that for driver dispatch handlers. Chip
On Thu, Feb 4, 2021 at 11:42 AM Chip Davis <cdavis(a)codeweavers.com> wrote:
February 4, 2021 10:40 AM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
@@ -1826,6 +1828,117 @@ static void query_property( struct disk_device *device, IRP *irp ) [...] +done: + LeaveCriticalSection( &device_section ); + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return STATUS_SUCCESS; +}
Shouldn't that return what's in io->u.Status? I thought MS recommended doing that for driver dispatch handlers.
I just copied what we were doing in dlls/mountmgr.sys/device.c:harddisk_ioctl, and it looks like it used to be that way and AJ changed it in 41eb2fd714c18422381f46e8b4f3608c06bef627. Do you happen to have a link to wherever you saw that recommendation? Best, Erich
February 4, 2021 12:59 PM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
On Thu, Feb 4, 2021 at 11:42 AM Chip Davis <cdavis(a)codeweavers.com> wrote:
February 4, 2021 10:40 AM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
@@ -1826,6 +1828,117 @@ static void query_property( struct disk_device *device, IRP *irp ) [...] +done: + LeaveCriticalSection( &device_section ); + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return STATUS_SUCCESS; +}
Shouldn't that return what's in io->u.Status? I thought MS recommended doing that for driver dispatch handlers.
I just copied what we were doing in dlls/mountmgr.sys/device.c:harddisk_ioctl, and it looks like it used to be that way and AJ changed it in 41eb2fd714c18422381f46e8b4f3608c06bef627. Do you happen to have a link to wherever you saw that recommendation?
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/different-w... See scenario 5.
Best, Erich
Chip
February 4, 2021 1:53 PM, "Chip Davis" <cdavis(a)codeweavers.com> wrote:
February 4, 2021 12:59 PM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
On Thu, Feb 4, 2021 at 11:42 AM Chip Davis <cdavis(a)codeweavers.com> wrote:
February 4, 2021 10:40 AM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
@@ -1826,6 +1828,117 @@ static void query_property( struct disk_device *device, IRP *irp ) [...] +done: + LeaveCriticalSection( &device_section ); + IoCompleteRequest( irp, IO_NO_INCREMENT ); + return STATUS_SUCCESS; +}
Shouldn't that return what's in io->u.Status? I thought MS recommended doing that for driver dispatch handlers.
I just copied what we were doing in dlls/mountmgr.sys/device.c:harddisk_ioctl, and it looks like it used to be that way and AJ changed it in 41eb2fd714c18422381f46e8b4f3608c06bef627. Do you happen to have a link to wherever you saw that recommendation?
https://docs.microsoft.com/en-us/windows-hardware/drivers/kernel/different-w... t-sheet
See scenario 5.
It looks like that change was intended to prevent accessing IRP memory after it might have been freed. The thing to do, then, is to copy the status out of the IOSB into a local variable before calling IoCompleteRequest(). Chip
On Thu, Feb 4, 2021 at 1:35 PM Chip Davis <cdavis(a)codeweavers.com> wrote:
... It looks like that change was intended to prevent accessing IRP memory after it might have been freed. The thing to do, then, is to copy the status out of the IOSB into a local variable before calling IoCompleteRequest().
That makes sense, I ran into a somewhat similar issue with a previous version of patch 2. I'll try to look into putting together an appropriate update for dlls/mountmgr.sys/device.c:harddisk_ioctl this evening, did you notice any other issues with this series? Best, Erich
February 4, 2021 2:52 PM, "Erich E. Hoover" <erich.e.hoover(a)gmail.com> wrote:
On Thu, Feb 4, 2021 at 1:35 PM Chip Davis <cdavis(a)codeweavers.com> wrote:
... It looks like that change was intended to prevent accessing IRP memory after it might have been freed. The thing to do, then, is to copy the status out of the IOSB into a local variable before calling IoCompleteRequest().
That makes sense, I ran into a somewhat similar issue with a previous version of patch 2. I'll try to look into putting together an appropriate update for dlls/mountmgr.sys/device.c:harddisk_ioctl this evening, did you notice any other issues with this series?
No. Chip
"Erich E. Hoover" <erich.e.hoover(a)gmail.com> writes:
On Thu, Feb 4, 2021 at 1:35 PM Chip Davis <cdavis(a)codeweavers.com> wrote:
... It looks like that change was intended to prevent accessing IRP memory after it might have been freed. The thing to do, then, is to copy the status out of the IOSB into a local variable before calling IoCompleteRequest().
That makes sense, I ran into a somewhat similar issue with a previous version of patch 2. I'll try to look into putting together an appropriate update for dlls/mountmgr.sys/device.c:harddisk_ioctl this evening, did you notice any other issues with this series?
Note that it shouldn't make any difference, because the IRP result has been sent to the server already. -- Alexandre Julliard julliard(a)winehq.org
February 4, 2021 3:02 PM, "Alexandre Julliard" <julliard(a)winehq.org> wrote:
"Erich E. Hoover" <erich.e.hoover(a)gmail.com> writes:
On Thu, Feb 4, 2021 at 1:35 PM Chip Davis <cdavis(a)codeweavers.com> wrote:
... It looks like that change was intended to prevent accessing IRP memory after it might have been freed. The thing to do, then, is to copy the status out of the IOSB into a local variable before calling IoCompleteRequest().
That makes sense, I ran into a somewhat similar issue with a previous version of patch 2. I'll try to look into putting together an appropriate update for dlls/mountmgr.sys/device.c:harddisk_ioctl this evening, did you notice any other issues with this series?
Note that it shouldn't make any difference, because the IRP result has been sent to the server already.
I have patches in staging which make the server use the dispatch result in the case of a non-overlapped I/O. This is intended to fix drivers like SafeDisc that assume that the dispatch result and not the IOSB result will be returned. They're disabled in part because of this issue. Chip
participants (3)
-
Alexandre Julliard -
Chip Davis -
Erich E. Hoover