Hi GaoKui, On Thu, Jul 26, 2018 at 02:44:30AM +0000, gaokui (A) wrote: > Hi, Eric, > Thanks for your reply. > > I have run your program on an original kernel and it reproduced the crash. And I also run the program on a kernel with our patch, but there was no crash. > > I think the reason of the crash is the parameter buffer is aligned with the page . So the address of the parameter buffer starts at the beginning of the page, which making "walk->offset = 0" and generating the crash. I add some logs in "scatterwalk_pagedone()" to print the value of walk->offset, and the log before the crash shows that "walk->offset = 0". > > And I do not understand why "walk->offset = 0" means no data to be processed. In the structure " scatterlist", the member "offset" represents the offset of the buffer in the page, and the member length represents the length of the buffer. In function "af_alg_make_sg()", if a buffer occupies more than one pages, the offset will also be set to 0 in the second and following pages. And In function scatterwalk_done(), walk->offset = 0 will also allow to call "scatterwalk_pagedone()". So I think that when "walk->offset = 0" the page needs to be flushed as well. > > BRs > GaoKui > Did you test my patches or just yours? Your patch fixes the crash, but I don't agree that it's the best fix. What you're missing is that walk->offset has already been increased by scatterwalk_advance() to the offset of the *end* of the data region processed. Hence, walk->offset = 0 implies that 0 bytes were processed (as walk->offset must have been 0 initially, then had 0 added to it), which I think isn't meant to be a valid case. And in particular it does *not* make sense to flush any page when 0 bytes were processed. Note that this could also be a problem for empty scatterlist elements, but AFAICS the scatterlist walk code doesn't actually support those when the total length isn't 0. I think that needs improvement too, but AFAICS other changes would be needed to properly fix that limitation, and you apparently cannot generate empty scatterlist elements via AF_ALG anyway so only in-kernel users would be affected. - Eric
