Re: [Cryptodev-linux-devel] [PATCH] fix the NULL input or output case for get_userbuf

[Phil Sutter]

Hi Cristian,

On Wed, Jan 20, 2016 at 02:06:49PM +, Cristian Stoica wrote:
> > I really have no idea where you're heading at with this. But without further
> > context, it doesn't make any sense to me. If you have a functional
> > enhancement up your sleeve, I suggest submitting that along with this
> > change so I get an idea of what this is all about.
> > 
> 
> Of course, these patches have some context.
> 
> I'm playing with the code to speed up hashing operations.
> For now I want to see if there is a simple design that will make better use 
> of the Linux Crypto API which has both 
> 'init-update-final' and 'digest' operations.
> One objective is to have less ioctl calls and avoid unnecessary init-updates 
> where a single digest will do.

Ah, this explains things a bit.

> Currently I have a CIOCHASH ioctl that is identical to CIOCCRYPT but only for 
> hashing. In the current form it is faster for my test cases but needs some 
> clarifications on API.
> 
> I can send some code to discuss design ideas if you are interested.

Sure, just send them as RFC to the list and we'll figure things out
together. :)

Thanks, Phil

___
Cryptodev-linux-devel mailing list
Cryptodev-linux-devel@gna.org
https://mail.gna.org/listinfo/cryptodev-linux-devel

Re: [Cryptodev-linux-devel] [PATCH] fix the NULL input or output case for get_userbuf

[Cristian Stoica]

On 01/18/2016 11:04 PM, Phil Sutter wrote:
> On Tue, Jan 12, 2016 at 12:08:02PM +0200, Cristian Stoica wrote:
...
>> -*src_sg = NULL; /* default to no input */
>> -*dst_sg = NULL; /* default to ignore output */
>> -

>
> This does not look correct to me. The purpose of those is to ensure
> *src_sg and *dst_sg are initialized. Otherwise if src or dst are NULL
> (which matches the situation of "NULL input or output" as you state),
> they are left uninitialized by this function.

> Do you have an actual use case where this bites you?

The use-case I'm thinking is one where get_userbuf is called knowing upfront 
that
only one of src or dst are of interest, skipping work completely on the other 
buffer.
Of course, I can call get_userbuf with both output scatterlists and ignore one 
of
them. But I would add more flexibility to this function if it is possible.

For example:
ret = get_userbuf(ses_ptr, src, len, NULL, 0, task, mm, _sg, NULL);
    ^ 
The current implementation does not permit output  to be NULL even 
though
it allows for input buffer to be NULL.


If this patch is incorrect for all cases, I'm thinking of some alternative 
implementations:
- the callers set the scatterlists  with NULL before calling get_userbuf
- get_userbuf checks for non-NULL output scatterlists before writing default 
values to them


Cristian S.






___
Cryptodev-linux-devel mailing list
Cryptodev-linux-devel@gna.org
https://mail.gna.org/listinfo/cryptodev-linux-devel

Re: [Cryptodev-linux-devel] [PATCH] fix the NULL input or output case for get_userbuf

[Cristian Stoica]

On 01/18/2016 11:04 PM, Phil Sutter wrote:
> On Tue, Jan 12, 2016 at 12:08:02PM +0200, Cristian Stoica wrote:
...
>> -*src_sg = NULL; /* default to no input */
>> -*dst_sg = NULL; /* default to ignore output */
>> -

>
> This does not look correct to me. The purpose of those is to ensure
> *src_sg and *dst_sg are initialized. Otherwise if src or dst are NULL
> (which matches the situation of "NULL input or output" as you state),
> they are left uninitialized by this function.

> Do you have an actual use case where this bites you?

The use-case I'm thinking is one where get_userbuf is called knowing upfront 
that
only one of src or dst are of interest, skipping work completely on the other 
buffer.
Of course, I can call get_userbuf with both output scatterlists and ignore one 
of
them. But I would add more flexibility to this function if it is possible.

For example:
ret = get_userbuf(ses_ptr, src, len, NULL, 0, task, mm, _sg, NULL);
    ^ 
The current implementation does not permit output  to be NULL even 
though
it allows for input buffer to be NULL.


If this patch is incorrect for all cases, I'm thinking of some alternative 
implementations:
- the callers set the scatterlists  with NULL before calling get_userbuf
- get_userbuf checks for non-NULL output scatterlists before writing default 
values to them


Cristian S.







___
Cryptodev-linux-devel mailing list
Cryptodev-linux-devel@gna.org
https://mail.gna.org/listinfo/cryptodev-linux-devel

Re: [Cryptodev-linux-devel] [PATCH] fix the NULL input or output case for get_userbuf

[Phil Sutter]

On Tue, Jan 19, 2016 at 01:38:53PM +, Cristian Stoica wrote:
> 
> 
> On 01/18/2016 11:04 PM, Phil Sutter wrote:
> > On Tue, Jan 12, 2016 at 12:08:02PM +0200, Cristian Stoica wrote:
> ...
> >> -  *src_sg = NULL; /* default to no input */
> >> -  *dst_sg = NULL; /* default to ignore output */
> >> -
> 
> >
> > This does not look correct to me. The purpose of those is to ensure
> > *src_sg and *dst_sg are initialized. Otherwise if src or dst are NULL
> > (which matches the situation of "NULL input or output" as you state),
> > they are left uninitialized by this function.
> 
> > Do you have an actual use case where this bites you?
> 
> The use-case I'm thinking is one where get_userbuf is called knowing upfront 
> that
> only one of src or dst are of interest, skipping work completely on the other 
> buffer.
> Of course, I can call get_userbuf with both output scatterlists and ignore 
> one of
> them. But I would add more flexibility to this function if it is possible.
> 
> For example:
> ret = get_userbuf(ses_ptr, src, len, NULL, 0, task, mm, _sg, NULL);
>     ^ 
> The current implementation does not permit output  to be NULL 
> even though
> it allows for input buffer to be NULL.

But it also doesn't need to, because get_userbuf() is never called this
way. In my opinion you're optimizing for a use-case which doesn't exist.
Once there is one, I'm all open for changes in that function, but until
that's the case I don't see sense in changing that function's behaviour.

> If this patch is incorrect for all cases, I'm thinking of some alternative 
> implementations:
> - the callers set the scatterlists  with NULL before calling get_userbuf
> - get_userbuf checks for non-NULL output scatterlists before writing default 
> values to them

It doesn't need to. The caller always just passes a pointer and expects
it to be initialized, either to NULL or a scatterlist which points to
the data src/dst points to. This is the determining information, and it
does the right thing in any case.

I really have no idea where you're heading at with this. But without
further context, it doesn't make any sense to me. If you have a
functional enhancement up your sleeve, I suggest submitting that along
with this change so I get an idea of what this is all about.

Cheers, Phil

___
Cryptodev-linux-devel mailing list
Cryptodev-linux-devel@gna.org
https://mail.gna.org/listinfo/cryptodev-linux-devel

Re: [Cryptodev-linux-devel] [PATCH] fix the NULL input or output case for get_userbuf

[Phil Sutter]

Hi,

On Tue, Jan 12, 2016 at 12:08:02PM +0200, Cristian Stoica wrote:
> NULL input or output is a valid case and we should not attempt
> to write default values to NULL input or output pointers.
> 
> Signed-off-by: Cristian Stoica 
> ---
>  zc.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/zc.c b/zc.c
> index 29b0501..d6adcc4 100644
> --- a/zc.c
> +++ b/zc.c
> @@ -176,9 +176,6 @@ int get_userbuf(struct csession *ses,
>   return 0;
>   }
>  
> - *src_sg = NULL; /* default to no input */
> - *dst_sg = NULL; /* default to ignore output */
> -

This does not look correct to me. The purpose of those is to ensure
*src_sg and *dst_sg are initialized. Otherwise if src or dst are NULL
(which matches the situation of "NULL input or output" as you state),
they are left uninitialized by this function.

Do you have an actual use case where this bites you?

Thanks, Phil

___
Cryptodev-linux-devel mailing list
Cryptodev-linux-devel@gna.org
https://mail.gna.org/listinfo/cryptodev-linux-devel