Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Peter Lieven]

Am 22.08.2014 um 09:35 schrieb Peter Lieven:
> Some code in the block layer makes potentially huge allocations. Failure
> is not completely unexpected there, so avoid aborting qemu and handle
> out-of-memory situations gracefully.
>
> This patch addresses the allocations in the iscsi block driver.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> Reviewed-by: Benoit Canet <benoit@irqsave.net>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
>  block/iscsi.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/block/iscsi.c b/block/iscsi.c
> index 84aa22a..06afa78 100644
> --- a/block/iscsi.c
> +++ b/block/iscsi.c
> @@ -893,7 +893,10 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState
> *bs, int64_t sector_num,
>      nb_blocks = sector_qemu2lun(nb_sectors, iscsilun);
>
>      if (iscsilun->zeroblock == NULL) {
> -        iscsilun->zeroblock = g_malloc0(iscsilun->block_size);
> +        iscsilun->zeroblock = g_try_malloc0(iscsilun->block_size);
> +        if (iscsilun->zeroblock == NULL) {
> +            return -ENOMEM;
> +        }
>      }
>
>      iscsi_co_init_iscsitask(iscsilun, &iTask);

Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
on the blocksize. What is significantly larger is the allocationmap. It is typically created
on iscsi_open, but is also recreated on iscsi_truncate. I don't have the context why this
patch was introduced, but I would vote for introducing a bitmap_try_new and issue
a warning if the allocation fails. The allocationmap is optional we can work without it.
If the pointer is NULL its not used.

Peter

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Kevin Wolf]

Am 22.08.2014 um 09:40 hat Peter Lieven geschrieben:
> Am 22.08.2014 um 09:35 schrieb Peter Lieven:
> > Some code in the block layer makes potentially huge allocations. Failure
> > is not completely unexpected there, so avoid aborting qemu and handle
> > out-of-memory situations gracefully.
> >
> > This patch addresses the allocations in the iscsi block driver.
> >
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> > Reviewed-by: Benoit Canet <benoit@irqsave.net>
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > ---
> >  block/iscsi.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/iscsi.c b/block/iscsi.c
> > index 84aa22a..06afa78 100644
> > --- a/block/iscsi.c
> > +++ b/block/iscsi.c
> > @@ -893,7 +893,10 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState
> > *bs, int64_t sector_num,
> >      nb_blocks = sector_qemu2lun(nb_sectors, iscsilun);
> >
> >      if (iscsilun->zeroblock == NULL) {
> > -        iscsilun->zeroblock = g_malloc0(iscsilun->block_size);
> > +        iscsilun->zeroblock = g_try_malloc0(iscsilun->block_size);
> > +        if (iscsilun->zeroblock == NULL) {
> > +            return -ENOMEM;
> > +        }
> >      }
> >
> >      iscsi_co_init_iscsitask(iscsilun, &iTask);
> 
> Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
> on the blocksize.

I don't remember the details, but I think when I went through all
drivers, I couldn't convince myself that a reasonable block size is
enforced somewhere. So I just went ahead and converted the call to be on
the safe side. It can never hurt anyway.

> What is significantly larger is the allocationmap. It is typically created
> on iscsi_open, but is also recreated on iscsi_truncate. I don't have the context why this
> patch was introduced, but I would vote for introducing a bitmap_try_new and issue
> a warning if the allocation fails. The allocationmap is optional we can work without it.
> If the pointer is NULL its not used.

Right, that one I missed because it doesn't directly use g_malloc().

Your proposal sounds good to me. Are you going to prepare a patch?

Kevin

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Peter Lieven]

Am 22.08.2014 um 10:42 schrieb Kevin Wolf:
> Am 22.08.2014 um 09:40 hat Peter Lieven geschrieben:
>> Am 22.08.2014 um 09:35 schrieb Peter Lieven:
>>> Some code in the block layer makes potentially huge allocations. Failure
>>> is not completely unexpected there, so avoid aborting qemu and handle
>>> out-of-memory situations gracefully.
>>>
>>> This patch addresses the allocations in the iscsi block driver.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
>>> Reviewed-by: Benoit Canet <benoit@irqsave.net>
>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>> ---
>>>  block/iscsi.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/iscsi.c b/block/iscsi.c
>>> index 84aa22a..06afa78 100644
>>> --- a/block/iscsi.c
>>> +++ b/block/iscsi.c
>>> @@ -893,7 +893,10 @@ coroutine_fn iscsi_co_write_zeroes(BlockDriverState
>>> *bs, int64_t sector_num,
>>>      nb_blocks = sector_qemu2lun(nb_sectors, iscsilun);
>>>
>>>      if (iscsilun->zeroblock == NULL) {
>>> -        iscsilun->zeroblock = g_malloc0(iscsilun->block_size);
>>> +        iscsilun->zeroblock = g_try_malloc0(iscsilun->block_size);
>>> +        if (iscsilun->zeroblock == NULL) {
>>> +            return -ENOMEM;
>>> +        }
>>>      }
>>>
>>>      iscsi_co_init_iscsitask(iscsilun, &iTask);
>> Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
>> on the blocksize.
> I don't remember the details, but I think when I went through all
> drivers, I couldn't convince myself that a reasonable block size is
> enforced somewhere. So I just went ahead and converted the call to be on
> the safe side. It can never hurt anyway.
>
>> What is significantly larger is the allocationmap. It is typically created
>> on iscsi_open, but is also recreated on iscsi_truncate. I don't have the context why this
>> patch was introduced, but I would vote for introducing a bitmap_try_new and issue
>> a warning if the allocation fails. The allocationmap is optional we can work without it.
>> If the pointer is NULL its not used.
> Right, that one I missed because it doesn't directly use g_malloc().
>
> Your proposal sounds good to me. Are you going to prepare a patch?

will do.

Peter

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Paolo Bonzini]

Il 22/08/2014 10:42, Kevin Wolf ha scritto:
> > Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
> > on the blocksize.
> 
> I don't remember the details, but I think when I went through all
> drivers, I couldn't convince myself that a reasonable block size is
> enforced somewhere. So I just went ahead and converted the call to be on
> the safe side. It can never hurt anyway.

Yeah, a malicious iSCSI target could have unreasonable block sizes.

This means the minimum transfer size for SCSI devices could be on the
order of half a GiB, and that could cause other unbounded allocations in
the read-modify-write code.  Are those protected too?

Paolo

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Peter Lieven]

Am 24.08.2014 um 18:30 schrieb Paolo Bonzini:
> Il 22/08/2014 10:42, Kevin Wolf ha scritto:
>>> Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
>>> on the blocksize.
>> I don't remember the details, but I think when I went through all
>> drivers, I couldn't convince myself that a reasonable block size is
>> enforced somewhere. So I just went ahead and converted the call to be on
>> the safe side. It can never hurt anyway.
> Yeah, a malicious iSCSI target could have unreasonable block sizes.
Maybe we should just allow 512b or 4kb blocksize and refuse
all other?

Peter

>
> This means the minimum transfer size for SCSI devices could be on the
> order of half a GiB, and that could cause other unbounded allocations in
> the read-modify-write code.  Are those protected too?
>
> Paolo

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Paolo Bonzini]

Il 24/08/2014 21:31, Peter Lieven ha scritto:
> Am 24.08.2014 um 18:30 schrieb Paolo Bonzini:
>> Il 22/08/2014 10:42, Kevin Wolf ha scritto:
>>>> Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
>>>> on the blocksize.
>>> I don't remember the details, but I think when I went through all
>>> drivers, I couldn't convince myself that a reasonable block size is
>>> enforced somewhere. So I just went ahead and converted the call to be on
>>> the safe side. It can never hurt anyway.
>> Yeah, a malicious iSCSI target could have unreasonable block sizes.
> Maybe we should just allow 512b or 4kb blocksize and refuse
> all other?

At least CDs and DVDs use 2kb blocksize. :)

Paolo

Re: [Qemu-devel] [Fwd: [PATCH v4 07/21] iscsi: Handle failure for potentially large allocations]

[Peter Lieven]

On 25.08.2014 10:56, Paolo Bonzini wrote:
> Il 24/08/2014 21:31, Peter Lieven ha scritto:
>> Am 24.08.2014 um 18:30 schrieb Paolo Bonzini:
>>> Il 22/08/2014 10:42, Kevin Wolf ha scritto:
>>>>> Unfortunately, I missed that one. The zeroblock is typicalls 512 Byte or 4K depending
>>>>> on the blocksize.
>>>> I don't remember the details, but I think when I went through all
>>>> drivers, I couldn't convince myself that a reasonable block size is
>>>> enforced somewhere. So I just went ahead and converted the call to be on
>>>> the safe side. It can never hurt anyway.
>>> Yeah, a malicious iSCSI target could have unreasonable block sizes.
>> Maybe we should just allow 512b or 4kb blocksize and refuse
>> all other?
> At least CDs and DVDs use 2kb blocksize. :)

power of 2 && <= 4096 ?

>
> Paolo
>