| Message ID | 20190610150317.29546-3-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | scsi: use sg helper to operate sgl | expand |
On Mon, 2019-06-10 at 23:03 +0800, Ming Lei wrote: > The current way isn't safe for chained sgl, so use sgl helper to > operate sgl. > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/scsi/advansys.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c > index 926311c792d5..a242a62caaa1 100644 > --- a/drivers/scsi/advansys.c > +++ b/drivers/scsi/advansys.c > @@ -7710,7 +7710,7 @@ adv_get_sglist(struct asc_board *boardp, adv_req_t *reqp, > sg_block->sg_ptr = 0L; /* Last ADV_SG_BLOCK in list. */ > return ADV_SUCCESS; > } > - slp++; > + slp = sg_next(slp); > } > sg_block->sg_cnt = NO_OF_SG_PER_BLOCK; > prev_sg_block = sg_block; Reviewed-by: Ewan D. Milne <emilne@redhat.com>
On Mon, 2019-06-10 at 23:03 +0800, Ming Lei wrote: > The current way isn't safe for chained sgl, so use sgl helper to > operate sgl. The advansys driver doesn't currently use a chained scatterlist. In theory it could; the if (shost->sg_tablesize > SG_ALL) { shost->sg_tablesize = SG_ALL; } At around line 11226 is what prevents it and that could be eliminated provided someone actually has the hardware to test. However, provided drivers make the correct SG_ALL or less declaration, they're entitled to treat scatterlists as fully contiguous, so there's no real justification (beyond uniformity) for making it use the chain helpers. James
On Mon, 2019-06-10 at 11:37 -0700, James Bottomley wrote: > On Mon, 2019-06-10 at 23:03 +0800, Ming Lei wrote: > > The current way isn't safe for chained sgl, so use sgl helper to > > operate sgl. > > The advansys driver doesn't currently use a chained scatterlist. In > theory it could; the > > if (shost->sg_tablesize > SG_ALL) { > shost->sg_tablesize = SG_ALL; > } > > At around line 11226 is what prevents it and that could be eliminated > provided someone actually has the hardware to test. > > However, provided drivers make the correct SG_ALL or less declaration, > they're entitled to treat scatterlists as fully contiguous, so there's > no real justification (beyond uniformity) for making it use the chain > helpers. > > James > I thought the whole issue came about because Ming's earlier changes to scsi_lib.c made the previously SG_CHUNK_SIZE scatterlist allocated with the struct request much smaller, (SCSI_INLINE_SG_CNT is 2) so everything needs to support it? -Ewan
On Mon, 2019-06-10 at 15:36 -0400, Ewan D. Milne wrote: > On Mon, 2019-06-10 at 11:37 -0700, James Bottomley wrote: > > On Mon, 2019-06-10 at 23:03 +0800, Ming Lei wrote: > > > The current way isn't safe for chained sgl, so use sgl helper to > > > operate sgl. > > > > The advansys driver doesn't currently use a chained > > scatterlist. In > > theory it could; the > > > > if (shost->sg_tablesize > SG_ALL) { > > shost->sg_tablesize = SG_ALL; > > } > > > > At around line 11226 is what prevents it and that could be > > eliminated provided someone actually has the hardware to test. > > > > However, provided drivers make the correct SG_ALL or less > > declaration, they're entitled to treat scatterlists as fully > > contiguous, so there's no real justification (beyond uniformity) > > for making it use the chain helpers. > > > > James > > > > I thought the whole issue came about because Ming's earlier changes > to scsi_lib.c made the previously SG_CHUNK_SIZE scatterlist allocated > with the struct request much smaller, (SCSI_INLINE_SG_CNT is 2) so > everything needs to support it? Um, well, I'm just going by the commit log. If the problem is someone broke the SG_ALL no chaining assumption in an earlier commit, finger that as the buggy commit you're fixing rather than implying the drivers had a longstanding bug. In fact, preferably do this work as a precursor to the original instead of leaving the drivers broken for a bisect. James
On Mon, Jun 10, 2019 at 04:02:27PM -0700, James Bottomley wrote: > On Mon, 2019-06-10 at 15:36 -0400, Ewan D. Milne wrote: > > On Mon, 2019-06-10 at 11:37 -0700, James Bottomley wrote: > > > On Mon, 2019-06-10 at 23:03 +0800, Ming Lei wrote: > > > > The current way isn't safe for chained sgl, so use sgl helper to > > > > operate sgl. > > > > > > The advansys driver doesn't currently use a chained > > > scatterlist. In > > > theory it could; the > > > > > > if (shost->sg_tablesize > SG_ALL) { > > > shost->sg_tablesize = SG_ALL; > > > } > > > > > > At around line 11226 is what prevents it and that could be > > > eliminated provided someone actually has the hardware to test. > > > > > > However, provided drivers make the correct SG_ALL or less > > > declaration, they're entitled to treat scatterlists as fully > > > contiguous, so there's no real justification (beyond uniformity) > > > for making it use the chain helpers. > > > > > > James > > > > > > > I thought the whole issue came about because Ming's earlier changes > > to scsi_lib.c made the previously SG_CHUNK_SIZE scatterlist allocated > > with the struct request much smaller, (SCSI_INLINE_SG_CNT is 2) so > > everything needs to support it? > > Um, well, I'm just going by the commit log. If the problem is someone > broke the SG_ALL no chaining assumption in an earlier commit, finger > that as the buggy commit you're fixing rather than implying the drivers > had a longstanding bug. In fact, preferably do this work as a > precursor to the original instead of leaving the drivers broken for a > bisect. Yeah, these drivers should have been converted from the beginning. Let's discuss how to move on now: 1) we introduce the following patches to avoid big pre-allocation since it won't be an accepted behaviour to consume GB maybe for nothing: scsi: core: avoid pre-allocating big SGL for data scsi: core: avoid pre-allocating big SGL for protection information scsi: lib/sg_pool.c: improve APIs for allocating sg pool 2) it is flexible to reserve a small SGL for avoiding runtime allocation, which needs driver to support chained SG. 3) I grep all scsi drivers, and found most of them are chained sgl ready, except for the 6 drivers in this patchset and esi_scsi which is fixed already. Given the above 3 patches are just in 5.3/scsi-queue, I am fine with either way: 1) revert the 3 first, then re-organize the whole patchset in correct order(convert drivers first, then the 3 above drivers) 2) simply apply the 5 patches now Any comments? Thanks, Ming
Ming, > 1) revert the 3 first, then re-organize the whole patchset in correct > order(convert drivers first, then the 3 above drivers) > > 2) simply apply the 5 patches now > > Any comments? I'm on the fence about this. Your patches were some of the first ones that went into the 5.3 tree. So I'd have to rebase pretty much the whole 5.3 queue. Whereas merging your updates leaves a sequence of 100+ commits that could lead to bisection problems in the future. I'm particularly worried about ipr and lpfc but all these drivers are actively used. As much as I like to see all drivers, without exception, use the sg iterators, it would have been nice to have a smoother transition.
Hi Martin, On Tue, Jun 11, 2019 at 07:50:01PM -0400, Martin K. Petersen wrote: > > Ming, > > > 1) revert the 3 first, then re-organize the whole patchset in correct > > order(convert drivers first, then the 3 above drivers) > > > > 2) simply apply the 5 patches now > > > > Any comments? > > I'm on the fence about this. Your patches were some of the first ones > that went into the 5.3 tree. So I'd have to rebase pretty much the whole > 5.3 queue. > > Whereas merging your updates leaves a sequence of 100+ commits that > could lead to bisection problems in the future. I'm particularly worried > about ipr and lpfc but all these drivers are actively used. The issue has been introduced, and people has complained, so I think we have to do something. > > As much as I like to see all drivers, without exception, use the sg > iterators, it would have been nice to have a smoother transition. All the 5 drivers are found via static code analysis by eyes, and not see other ways for looking at this issue. That said it is quite hard to prove 'all drivers, without exception, use the sg iterators'. Even though some of them is missed, it should have been triggered easily if drivers are actively used, then it can be fixed easily too. Thanks, Ming
On Wed, 2019-06-12 at 09:39 +0800, Ming Lei wrote: > Hi Martin, > > On Tue, Jun 11, 2019 at 07:50:01PM -0400, Martin K. Petersen wrote: > > > > Ming, > > > > > 1) revert the 3 first, then re-organize the whole patchset in > > > correct order(convert drivers first, then the 3 above drivers) > > > > > > 2) simply apply the 5 patches now > > > > > > Any comments? > > > > I'm on the fence about this. Your patches were some of the first > > ones that went into the 5.3 tree. So I'd have to rebase pretty much > > the whole 5.3 queue. > > > > Whereas merging your updates leaves a sequence of 100+ commits that > > could lead to bisection problems in the future. I'm particularly > > worried about ipr and lpfc but all these drivers are actively used. > > The issue has been introduced, and people has complained, so I think > we have to do something. Studying the issue further, I think we have to do the rebase. The problem is that any driver which hasn't been updated can be persuaded to walk of the end of the request and dereference the next struct request. It's not impossible for userspace to set up both requests, so it looks like this could be used at least to leak information from the kernel if not exploit it outright. I think that means we have to have every driver updated before this goes in. > > As much as I like to see all drivers, without exception, use the sg > > iterators, it would have been nice to have a smoother transition. > > All the 5 drivers are found via static code analysis by eyes, and not > see other ways for looking at this issue. Can't coccinelle be persuaded? All we're looking for is a semantic search where we have a struct scatterlist that is either incremented or indexed. That said, it looks like the microtek scanner is yet another driver that needs updating. James > That said it is quite hard to prove 'all drivers, without exception, > use the sg iterators'. > > Even though some of them is missed, it should have been triggered > easily if drivers are actively used, then it can be fixed easily too.
James, > Studying the issue further, I think we have to do the rebase. The > problem is that any driver which hasn't been updated can be persuaded > to walk of the end of the request and dereference the next struct > request. It's not impossible for userspace to set up both requests, > so it looks like this could be used at least to leak information from > the kernel if not exploit it outright. I think that means we have to > have every driver updated before this goes in. I agree in theory. Although, regardless of ordering of the commits, this would still be a single pull request for 5.3. So it's not like there would be a kernel release with this flaw exposed. Assuming all drivers get fixed. Hence my concerns about breaking bisection. Not in terms of being able to build, but in terms of being able to test intermediate commits on systems with the affected drivers. Ming: Please audit all drivers, including ones that live outside of drivers/scsi but which use the midlayer such a s390, USB, libata, etc. Just to make sure we've got all of them covered. And then I think I'm inclined to reorder the 5.3 queue.
On Tue, Jun 11, 2019 at 11:09:51PM -0400, Martin K. Petersen wrote: > > James, > > > Studying the issue further, I think we have to do the rebase. The > > problem is that any driver which hasn't been updated can be persuaded > > to walk of the end of the request and dereference the next struct > > request. It's not impossible for userspace to set up both requests, > > so it looks like this could be used at least to leak information from > > the kernel if not exploit it outright. I think that means we have to > > have every driver updated before this goes in. > > I agree in theory. Although, regardless of ordering of the commits, this > would still be a single pull request for 5.3. So it's not like there > would be a kernel release with this flaw exposed. Assuming all drivers > get fixed. > > Hence my concerns about breaking bisection. Not in terms of being able > to build, but in terms of being able to test intermediate commits on > systems with the affected drivers. > > Ming: Please audit all drivers, including ones that live outside of > drivers/scsi but which use the midlayer such a s390, USB, libata, > etc. Just to make sure we've got all of them covered. OK, I am studying coccinelle, and should figure out one semantic patch for covering all these drivers. Thanks, Ming
On Wed, Jun 12, 2019 at 11:34:41AM +0800, Ming Lei wrote: > On Tue, Jun 11, 2019 at 11:09:51PM -0400, Martin K. Petersen wrote: > > > > James, > > > > > Studying the issue further, I think we have to do the rebase. The > > > problem is that any driver which hasn't been updated can be persuaded > > > to walk of the end of the request and dereference the next struct > > > request. It's not impossible for userspace to set up both requests, > > > so it looks like this could be used at least to leak information from > > > the kernel if not exploit it outright. I think that means we have to > > > have every driver updated before this goes in. > > > > I agree in theory. Although, regardless of ordering of the commits, this > > would still be a single pull request for 5.3. So it's not like there > > would be a kernel release with this flaw exposed. Assuming all drivers > > get fixed. > > > > Hence my concerns about breaking bisection. Not in terms of being able > > to build, but in terms of being able to test intermediate commits on > > systems with the affected drivers. > > > > Ming: Please audit all drivers, including ones that live outside of > > drivers/scsi but which use the midlayer such a s390, USB, libata, > > etc. Just to make sure we've got all of them covered. > > OK, I am studying coccinelle, and should figure out one semantic patch > for covering all these drivers. Looks the following semantic patch is working, if you are fine with it, I will start to work out patches with this coccinelle semantic path: @@ struct scatterlist *p; @@ ( - p++ + p = sg_next(p) | - p-- + p = sg_non_exist_prev(p) | - p += 1 + p = sg_next(p) | - p -= 1 + p = sg_non_exist_prev(p) | - p = p + 1 + p = sg_next(p) | - p = p - 1 + p = sg_non_exit_prev(p) ) @@ struct scatterlist *p; expression data != 0; @@ - p[data] + '!!!!!!use sg iterator helper!!!!!!' @@ struct scatterlist[] p; expression data != 0; @@ - p[data] + '!!!!!!use sg iterator helper!!!!!!' Thanks, Ming
diff --git a/drivers/scsi/advansys.c b/drivers/scsi/advansys.c index 926311c792d5..a242a62caaa1 100644 --- a/drivers/scsi/advansys.c +++ b/drivers/scsi/advansys.c @@ -7710,7 +7710,7 @@ adv_get_sglist(struct asc_board *boardp, adv_req_t *reqp, sg_block->sg_ptr = 0L; /* Last ADV_SG_BLOCK in list. */ return ADV_SUCCESS; } - slp++; + slp = sg_next(slp); } sg_block->sg_cnt = NO_OF_SG_PER_BLOCK; prev_sg_block = sg_block;
The current way isn't safe for chained sgl, so use sgl helper to operate sgl. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/advansys.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)