| Message ID | 1483538377-19379-2-git-send-email-maxg@mellanox.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
----- Original Message ----- > From: "Max Gurtovoy" <maxg@mellanox.com> > To: "Bart VanAssche" <Bart.VanAssche@sandisk.com>, dledford@redhat.com, linux-rdma@vger.kernel.org > Cc: "Israel Rukshin" <israelr@mellanox.com>, "Max Gurtovoy" <maxg@mellanox.com> > Sent: Wednesday, January 4, 2017 8:59:37 AM > Subject: [PATCHv2 1/1] IB/srp: fix invalid indirect_sg_entries parameter value > > From: Israel Rukshin <israelr@mellanox.com> > > After setting indirect_sg_entries module_param to huge value (e.g 500,000), > srp_alloc_req_data() fails to allocate indirect descriptors for the request > ring (kmalloc fails). This commit enforces the maximum value of > indirect_sg_entries > to be SG_MAX_SEGMENTS as signified in module param description. > > Signed-off-by: Israel Rukshin <israelr@mellanox.com> > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c > b/drivers/infiniband/ulp/srp/ib_srp.c > index 0f67cf9..79bf484 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -3699,6 +3699,12 @@ static int __init srp_init_module(void) > indirect_sg_entries = cmd_sg_entries; > } > > + if (indirect_sg_entries > SG_MAX_SEGMENTS) { > + pr_warn("Clamping indirect_sg_entries to %u\n", > + SG_MAX_SEGMENTS); > + indirect_sg_entries = SG_MAX_SEGMENTS; > + } > + > srp_remove_wq = create_workqueue("srp_remove"); > if (!srp_remove_wq) { > ret = -ENOMEM; > -- > 1.8.4.3 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-rdma" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Hello Max MODULE_PARM_DESC(indirect_sg_entries, "Default max number of gather/scatter entries (default is 12, max is " __stringify(SG_MAX_SEGMENTS) ")"); I am looking at the code here and SG_MAX_SEGMENTS is #define 2048 so it indeed caps the maximum size to 2048. The patch makes sense to now me as I see that the value for indirect_sg_entries was never controlled before. Of course if you behave and go by the modinfo you should never be setting it higher than 2048 but its good to manage the size. Looks good to me. Reviewed-by: Laurence Oberman <loberman@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-01-04 at 15:59 +0200, Max Gurtovoy wrote: > From: Israel Rukshin <israelr@mellanox.com> > > After setting indirect_sg_entries module_param to huge value (e.g 500,000), > srp_alloc_req_data() fails to allocate indirect descriptors for the request > ring (kmalloc fails). This commit enforces the maximum value of indirect_sg_entries > to be SG_MAX_SEGMENTS as signified in module param description. > > Signed-off-by: Israel Rukshin <israelr@mellanox.com> > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > --- > drivers/infiniband/ulp/srp/ib_srp.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c > index 0f67cf9..79bf484 100644 > --- a/drivers/infiniband/ulp/srp/ib_srp.c > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > @@ -3699,6 +3699,12 @@ static int __init srp_init_module(void) > indirect_sg_entries = cmd_sg_entries; > } > > + if (indirect_sg_entries > SG_MAX_SEGMENTS) { > + pr_warn("Clamping indirect_sg_entries to %u\n", > + SG_MAX_SEGMENTS); > + indirect_sg_entries = SG_MAX_SEGMENTS; > + } > + > srp_remove_wq = create_workqueue("srp_remove"); > if (!srp_remove_wq) { > ret = -ENOMEM; Thanks! Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com>-- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2017-01-04 at 15:09 +0000, Bart Van Assche wrote: > On Wed, 2017-01-04 at 15:59 +0200, Max Gurtovoy wrote: > > > > From: Israel Rukshin <israelr@mellanox.com> > > > > After setting indirect_sg_entries module_param to huge value (e.g > > 500,000), > > srp_alloc_req_data() fails to allocate indirect descriptors for the > > request > > ring (kmalloc fails). This commit enforces the maximum value of > > indirect_sg_entries > > to be SG_MAX_SEGMENTS as signified in module param description. > > > > Signed-off-by: Israel Rukshin <israelr@mellanox.com> > > Signed-off-by: Max Gurtovoy <maxg@mellanox.com> > > --- > > drivers/infiniband/ulp/srp/ib_srp.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/infiniband/ulp/srp/ib_srp.c > > b/drivers/infiniband/ulp/srp/ib_srp.c > > index 0f67cf9..79bf484 100644 > > --- a/drivers/infiniband/ulp/srp/ib_srp.c > > +++ b/drivers/infiniband/ulp/srp/ib_srp.c > > @@ -3699,6 +3699,12 @@ static int __init srp_init_module(void) > > indirect_sg_entries = cmd_sg_entries; > > } > > > > + if (indirect_sg_entries > SG_MAX_SEGMENTS) { > > + pr_warn("Clamping indirect_sg_entries to %u\n", > > + SG_MAX_SEGMENTS); > > + indirect_sg_entries = SG_MAX_SEGMENTS; > > + } > > + > > srp_remove_wq = create_workqueue("srp_remove"); > > if (!srp_remove_wq) { > > ret = -ENOMEM; > > Thanks! > > Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> This patch is a fix patch and should have been tagged as such. However, the proper tagging is complicated by the fact that the SG_MAX_SEGMENTS was originally named SCSI_MAX_SG_CHAIN_SEGMENTS. The fix applies all the way back to the first patch to enable SG segments in the srp driver, but you will need a different patch for the kernels prior to the rename. For this patch, I've added the following tags and applied the patch: Fixes: 65e8617fba17 (scsi: rename SCSI_MAX_{SG, SG_CHAIN}_SEGMENTS) Fixes: c07d424d6118 (IB/srp: add support for indirect tables that don't fit in SRP_CMD) Cc: stable@vger.kernel.org # 4.7+ Someone should write a patch for the earlier kernels and submit it directly to stable@ with the patch flagged for application on kernels from 2.6.39 to 4.6. The upstream hash for the fix to mainline is: commit 0a475ef4226e305bdcffe12b401ca1eab06c4913 Author: Israel Rukshin <israelr@mellanox.com> Date: Wed Jan 4 15:59:37 2017 +0200 IB/srp: fix invalid indirect_sg_entries parameter value
On 1/24/2017 6:32 PM, Doug Ledford wrote: > On Wed, 2017-01-04 at 15:09 +0000, Bart Van Assche wrote: >> On Wed, 2017-01-04 at 15:59 +0200, Max Gurtovoy wrote: >>> >>> From: Israel Rukshin <israelr@mellanox.com> >>> >>> After setting indirect_sg_entries module_param to huge value (e.g >>> 500,000), >>> srp_alloc_req_data() fails to allocate indirect descriptors for the >>> request >>> ring (kmalloc fails). This commit enforces the maximum value of >>> indirect_sg_entries >>> to be SG_MAX_SEGMENTS as signified in module param description. >>> >>> Signed-off-by: Israel Rukshin <israelr@mellanox.com> >>> Signed-off-by: Max Gurtovoy <maxg@mellanox.com> >>> --- >>> drivers/infiniband/ulp/srp/ib_srp.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c >>> b/drivers/infiniband/ulp/srp/ib_srp.c >>> index 0f67cf9..79bf484 100644 >>> --- a/drivers/infiniband/ulp/srp/ib_srp.c >>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c >>> @@ -3699,6 +3699,12 @@ static int __init srp_init_module(void) >>> indirect_sg_entries = cmd_sg_entries; >>> } >>> >>> + if (indirect_sg_entries > SG_MAX_SEGMENTS) { >>> + pr_warn("Clamping indirect_sg_entries to %u\n", >>> + SG_MAX_SEGMENTS); >>> + indirect_sg_entries = SG_MAX_SEGMENTS; >>> + } >>> + >>> srp_remove_wq = create_workqueue("srp_remove"); >>> if (!srp_remove_wq) { >>> ret = -ENOMEM; >> >> Thanks! >> >> Reviewed-by: Bart Van Assche <bart.vanassche@sandisk.com> > > This patch is a fix patch and should have been tagged as such. > However, the proper tagging is complicated by the fact that the > SG_MAX_SEGMENTS was originally named SCSI_MAX_SG_CHAIN_SEGMENTS. The > fix applies all the way back to the first patch to enable SG segments > in the srp driver, but you will need a different patch for the kernels > prior to the rename. For this patch, I've added the following tags and > applied the patch: > > Fixes: 65e8617fba17 (scsi: rename SCSI_MAX_{SG, SG_CHAIN}_SEGMENTS) > Fixes: c07d424d6118 (IB/srp: add support for indirect tables that don't > fit in SRP_CMD) > Cc: stable@vger.kernel.org # 4.7+ > > Someone should write a patch for the earlier kernels and submit it > directly to stable@ with the patch flagged for application on kernels > from 2.6.39 to 4.6. The upstream hash for the fix to mainline is: > > commit 0a475ef4226e305bdcffe12b401ca1eab06c4913 > Author: Israel Rukshin <israelr@mellanox.com> > Date: Wed Jan 4 15:59:37 2017 +0200 > > IB/srp: fix invalid indirect_sg_entries parameter value > Israel, please take it for earlier kernels as well. Max. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c index 0f67cf9..79bf484 100644 --- a/drivers/infiniband/ulp/srp/ib_srp.c +++ b/drivers/infiniband/ulp/srp/ib_srp.c @@ -3699,6 +3699,12 @@ static int __init srp_init_module(void) indirect_sg_entries = cmd_sg_entries; } + if (indirect_sg_entries > SG_MAX_SEGMENTS) { + pr_warn("Clamping indirect_sg_entries to %u\n", + SG_MAX_SEGMENTS); + indirect_sg_entries = SG_MAX_SEGMENTS; + } + srp_remove_wq = create_workqueue("srp_remove"); if (!srp_remove_wq) { ret = -ENOMEM;