* [PATCH 1/2] scatterlist: make sgl_free null pointer safe @ 2019-09-20 18:18 Sagi Grimberg 2019-09-20 18:18 ` [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl Sagi Grimberg 2019-09-22 5:33 ` [PATCH 1/2] scatterlist: make sgl_free null pointer safe Chaitanya Kulkarni 0 siblings, 2 replies; 6+ messages in thread From: Sagi Grimberg @ 2019-09-20 18:18 UTC (permalink / raw) To: linux-nvme; +Cc: Keith Busch, Christoph Hellwig Match other alloc/free routines by checking the sgl pointer first. Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- lib/scatterlist.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/scatterlist.c b/lib/scatterlist.c index c2cf2c311b7d..fea25b035e67 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -563,6 +563,9 @@ void sgl_free_n_order(struct scatterlist *sgl, int nents, int order) struct page *page; int i; + if (!sgl) + return; + for_each_sg(sgl, sg, nents, i) { if (!sg) break; -- 2.17.1 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl 2019-09-20 18:18 [PATCH 1/2] scatterlist: make sgl_free null pointer safe Sagi Grimberg @ 2019-09-20 18:18 ` Sagi Grimberg 2019-09-22 5:37 ` Chaitanya Kulkarni 2019-09-27 21:22 ` Christoph Hellwig 2019-09-22 5:33 ` [PATCH 1/2] scatterlist: make sgl_free null pointer safe Chaitanya Kulkarni 1 sibling, 2 replies; 6+ messages in thread From: Sagi Grimberg @ 2019-09-20 18:18 UTC (permalink / raw) To: linux-nvme; +Cc: Keith Busch, Christoph Hellwig Now that sgl_free is null safe, drop the superflous check. Signed-off-by: Sagi Grimberg <sagi@grimberg.me> --- drivers/nvme/target/tcp.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c index bf4f03474e89..d535080b781f 100644 --- a/drivers/nvme/target/tcp.c +++ b/drivers/nvme/target/tcp.c @@ -348,8 +348,7 @@ static int nvmet_tcp_map_data(struct nvmet_tcp_cmd *cmd) return 0; err: - if (cmd->req.sg_cnt) - sgl_free(cmd->req.sg); + sgl_free(cmd->req.sg); return NVME_SC_INTERNAL; } @@ -554,8 +553,7 @@ static int nvmet_try_send_data(struct nvmet_tcp_cmd *cmd) if (queue->nvme_sq.sqhd_disabled) { kfree(cmd->iov); - if (cmd->req.sg_cnt) - sgl_free(cmd->req.sg); + sgl_free(cmd->req.sg); } return 1; @@ -586,8 +584,7 @@ static int nvmet_try_send_response(struct nvmet_tcp_cmd *cmd, return -EAGAIN; kfree(cmd->iov); - if (cmd->req.sg_cnt) - sgl_free(cmd->req.sg); + sgl_free(cmd->req.sg); cmd->queue->snd_cmd = NULL; nvmet_tcp_put_cmd(cmd); return 1; @@ -1310,8 +1307,7 @@ static void nvmet_tcp_finish_cmd(struct nvmet_tcp_cmd *cmd) nvmet_req_uninit(&cmd->req); nvmet_tcp_unmap_pdu_iovec(cmd); kfree(cmd->iov); - if (cmd->req.sg_cnt) - sgl_free(cmd->req.sg); + sgl_free(cmd->req.sg); } static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue) -- 2.17.1 _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl 2019-09-20 18:18 ` [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl Sagi Grimberg @ 2019-09-22 5:37 ` Chaitanya Kulkarni 2019-09-27 21:22 ` Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Chaitanya Kulkarni @ 2019-09-22 5:37 UTC (permalink / raw) To: Sagi Grimberg, linux-nvme; +Cc: Keith Busch, Christoph Hellwig Please have a look at the comment at patch #1. If you look at the nvmet_req_free_sgl() we don't have such check since I think code assumes that sgl_free() is null safe. If that is correct we don't need additional checks in the code anyway Please correct me if I'm wrong. On 09/20/2019 11:18 AM, Sagi Grimberg wrote: > Now that sgl_free is null safe, drop the superflous check. > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > drivers/nvme/target/tcp.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c > index bf4f03474e89..d535080b781f 100644 > --- a/drivers/nvme/target/tcp.c > +++ b/drivers/nvme/target/tcp.c > @@ -348,8 +348,7 @@ static int nvmet_tcp_map_data(struct nvmet_tcp_cmd *cmd) > > return 0; > err: > - if (cmd->req.sg_cnt) > - sgl_free(cmd->req.sg); > + sgl_free(cmd->req.sg); > return NVME_SC_INTERNAL; > } > > @@ -554,8 +553,7 @@ static int nvmet_try_send_data(struct nvmet_tcp_cmd *cmd) > > if (queue->nvme_sq.sqhd_disabled) { > kfree(cmd->iov); > - if (cmd->req.sg_cnt) > - sgl_free(cmd->req.sg); > + sgl_free(cmd->req.sg); > } > > return 1; > @@ -586,8 +584,7 @@ static int nvmet_try_send_response(struct nvmet_tcp_cmd *cmd, > return -EAGAIN; > > kfree(cmd->iov); > - if (cmd->req.sg_cnt) > - sgl_free(cmd->req.sg); > + sgl_free(cmd->req.sg); > cmd->queue->snd_cmd = NULL; > nvmet_tcp_put_cmd(cmd); > return 1; > @@ -1310,8 +1307,7 @@ static void nvmet_tcp_finish_cmd(struct nvmet_tcp_cmd *cmd) > nvmet_req_uninit(&cmd->req); > nvmet_tcp_unmap_pdu_iovec(cmd); > kfree(cmd->iov); > - if (cmd->req.sg_cnt) > - sgl_free(cmd->req.sg); > + sgl_free(cmd->req.sg); > } > > static void nvmet_tcp_uninit_data_in_cmds(struct nvmet_tcp_queue *queue) > _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl 2019-09-20 18:18 ` [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl Sagi Grimberg 2019-09-22 5:37 ` Chaitanya Kulkarni @ 2019-09-27 21:22 ` Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2019-09-27 21:22 UTC (permalink / raw) To: Sagi Grimberg; +Cc: Keith Busch, Christoph Hellwig, linux-nvme On Fri, Sep 20, 2019 at 11:18:18AM -0700, Sagi Grimberg wrote: > Now that sgl_free is null safe, drop the superflous check. > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de> _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] scatterlist: make sgl_free null pointer safe 2019-09-20 18:18 [PATCH 1/2] scatterlist: make sgl_free null pointer safe Sagi Grimberg 2019-09-20 18:18 ` [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl Sagi Grimberg @ 2019-09-22 5:33 ` Chaitanya Kulkarni 2019-09-23 17:05 ` Sagi Grimberg 1 sibling, 1 reply; 6+ messages in thread From: Chaitanya Kulkarni @ 2019-09-22 5:33 UTC (permalink / raw) To: Sagi Grimberg, linux-nvme; +Cc: Keith Busch, Christoph Hellwig Unless I totally missed what you are trying to fix here, I think this case is already handled implicitly to avoid the code repetition inside for_each_sg() which makes :- sgl_free() sgl_free_order() sgl_free_n_order() code path NULL safe. I did a quick test without applying your patch with some prints and it worked file following module:- #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt #include <linux/init.h> #include <linux/blkdev.h> #include <linux/slab.h> #include <linux/module.h> #include <linux/kernel.h> int init_module(void) { sgl_free(NULL); return 0; } void cleanup_module(void) { } MODULE_LICENSE("GPL"); MODULE_VERSION("1.0"); Prints :- diff --git a/lib/scatterlist.c b/lib/scatterlist.c index c2cf2c3..56fa586 100644 --- a/lib/scatterlist.c +++ b/lib/scatterlist.c @@ -563,13 +563,20 @@ void sgl_free_n_order(struct scatterlist *sgl, int nents, int order) struct page *page; int i; + pr_info("%s %d\n", __func__, __LINE__); for_each_sg(sgl, sg, nents, i) { + pr_info("%s %d\n", __func__, __LINE__); if (!sg) break; + + pr_info("%s %d\n", __func__, __LINE__); page = sg_page(sg); if (page) __free_pages(page, order); + pr_info("%s %d\n", __func__, __LINE__); } + + pr_info("%s %d\n", __func__, __LINE__); kfree(sgl); } EXPORT_SYMBOL(sgl_free_n_order); @@ -581,6 +588,7 @@ void sgl_free_n_order(struct scatterlist *sgl, int nents, int order) */ void sgl_free_order(struct scatterlist *sgl, int order) { + pr_info("%s %d\n", __func__, __LINE__); sgl_free_n_order(sgl, INT_MAX, order); } EXPORT_SYMBOL(sgl_free_order); @@ -591,6 +599,7 @@ void sgl_free_order(struct scatterlist *sgl, int order) */ void sgl_free(struct scatterlist *sgl) { + pr_info("%s %d\n", __func__, __LINE__); sgl_free_order(sgl, 0); } EXPORT_SYMBOL(sgl_free); Got the following output without any crash or warning :- [ 156.950073] sgl_free 602 [ 156.950076] sgl_free_order 591 [ 156.950078] sgl_free_n_order 566 [ 156.950079] sgl_free_n_order 568 [ 156.950081] sgl_free_n_order 579 Did I miss anything ? On 09/20/2019 11:18 AM, Sagi Grimberg wrote: > Match other alloc/free routines by checking the sgl pointer first. > > Signed-off-by: Sagi Grimberg <sagi@grimberg.me> > --- > lib/scatterlist.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/lib/scatterlist.c b/lib/scatterlist.c > index c2cf2c311b7d..fea25b035e67 100644 > --- a/lib/scatterlist.c > +++ b/lib/scatterlist.c > @@ -563,6 +563,9 @@ void sgl_free_n_order(struct scatterlist *sgl, int nents, int order) > struct page *page; > int i; > > + if (!sgl) > + return; > + > for_each_sg(sgl, sg, nents, i) { > if (!sg) > break; > _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] scatterlist: make sgl_free null pointer safe 2019-09-22 5:33 ` [PATCH 1/2] scatterlist: make sgl_free null pointer safe Chaitanya Kulkarni @ 2019-09-23 17:05 ` Sagi Grimberg 0 siblings, 0 replies; 6+ messages in thread From: Sagi Grimberg @ 2019-09-23 17:05 UTC (permalink / raw) To: Chaitanya Kulkarni, linux-nvme; +Cc: Keith Busch, Christoph Hellwig > Unless I totally missed what you are trying to fix here, > I think this case is already handled implicitly to avoid > the code repetition inside for_each_sg() which makes :- You're right, the condition in the loop makes it null safe already. So we can safely disregard the first patch. _______________________________________________ Linux-nvme mailing list Linux-nvme@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-nvme ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-09-27 21:23 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-20 18:18 [PATCH 1/2] scatterlist: make sgl_free null pointer safe Sagi Grimberg 2019-09-20 18:18 ` [PATCH 2/2] nvmet-tcp: remove superflous check on request sgl Sagi Grimberg 2019-09-22 5:37 ` Chaitanya Kulkarni 2019-09-27 21:22 ` Christoph Hellwig 2019-09-22 5:33 ` [PATCH 1/2] scatterlist: make sgl_free null pointer safe Chaitanya Kulkarni 2019-09-23 17:05 ` Sagi Grimberg
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).