[PATCH resend v5 4/5] mountmgr: Add support for volume information queries.

[Erich E. Hoover]

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

[Chip Davis]

February 4, 2021 10:40 AM, "Erich E. Hoover"  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

[Erich E. Hoover]

On Thu, Feb 4, 2021 at 11:42 AM Chip Davis  wrote:
> February 4, 2021 10:40 AM, "Erich E. Hoover"  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

[Chip Davis]

February 4, 2021 12:59 PM, "Erich E. Hoover"  wrote:
> On Thu, Feb 4, 2021 at 11:42 AM Chip Davis  wrote:
> > February 4, 2021 10:40 AM, "Erich E. Hoover"  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

[Chip Davis]

February 4, 2021 1:53 PM, "Chip Davis"  wrote:
> February 4, 2021 12:59 PM, "Erich E. Hoover"  wrote:
> > On Thu, Feb 4, 2021 at 11:42 AM Chip Davis  wrote:
> > > February 4, 2021 10:40 AM, "Erich E. Hoover"  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

[Erich E. Hoover]

On Thu, Feb 4, 2021 at 1:35 PM Chip Davis  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

[Chip Davis]

February 4, 2021 2:52 PM, "Erich E. Hoover"  wrote:
> On Thu, Feb 4, 2021 at 1:35 PM Chip Davis  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

[Alexandre Julliard]

"Erich E. Hoover"  writes:
> On Thu, Feb 4, 2021 at 1:35 PM Chip Davis  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.

[Chip Davis]

February 4, 2021 3:02 PM, "Alexandre Julliard"  wrote:
> "Erich E. Hoover"  writes:
> > On Thu, Feb 4, 2021 at 1:35 PM Chip Davis  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