Re: [Cryptodev-linux-devel] [PATCH] fix the NULL input or output case for get_userbuf
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
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
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
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
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