| Message ID | 20210405175452.37578-5-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/nvme: misc fixes | expand |
On 4/5/21 7:54 PM, Klaus Jensen wrote: > From: Klaus Jensen <k.jensen@samsung.com> > > The controller namespaces array being 0-indexed requires 'nsid - 1' > everywhere. Something that is easy to miss. Align the controller > namespaces array with the subsystem namespaces array such that both are > 1-indexed. TBH I don't understand the justification. Assuming you hit a bug and try to protect yourself, maybe now you should also check for assert(n->namespaces[0] == NULL); somewhere. In nvme_ns() maybe? > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> > --- > hw/block/nvme.h | 8 ++++---- > hw/block/nvme.c | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h > index 9edc86d79e98..c610ab30dc5c 100644 > --- a/hw/block/nvme.h > +++ b/hw/block/nvme.h > @@ -217,7 +217,7 @@ typedef struct NvmeCtrl { > * Attached namespaces to this controller. If subsys is not given, all > * namespaces in this list will always be attached. > */ > - NvmeNamespace *namespaces[NVME_MAX_NAMESPACES]; > + NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1]; > NvmeSQueue **sq; > NvmeCQueue **cq; > NvmeSQueue admin_sq; > @@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid) > return NULL; > } > > - return n->namespaces[nsid - 1]; > + return n->namespaces[nsid]; > } > > static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns) > @@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns) > uint32_t nsid = nvme_nsid(ns); > assert(nsid && nsid <= NVME_MAX_NAMESPACES); > > - n->namespaces[nsid - 1] = ns; > + n->namespaces[nsid] = ns; > } > > static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) > @@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) > uint32_t nsid = nvme_nsid(ns); > assert(nsid && nsid <= NVME_MAX_NAMESPACES); > > - n->namespaces[nsid - 1] = NULL; > + n->namespaces[nsid] = NULL; > } > > static inline NvmeCQueue *nvme_cq(NvmeRequest *req) > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > index c54ec3c9523c..6d31d5b17a0b 100644 > --- a/hw/block/nvme.c > +++ b/hw/block/nvme.c > @@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) > return -1; > } > } else { > - if (n->namespaces[nsid - 1]) { > + if (n->namespaces[nsid]) { > error_setg(errp, "namespace id '%d' is already in use", nsid); > return -1; > } >
On Apr 6 09:01, Philippe Mathieu-Daudé wrote: > On 4/5/21 7:54 PM, Klaus Jensen wrote: > > From: Klaus Jensen <k.jensen@samsung.com> > > > > The controller namespaces array being 0-indexed requires 'nsid - 1' > > everywhere. Something that is easy to miss. Align the controller > > namespaces array with the subsystem namespaces array such that both are > > 1-indexed. > > TBH I don't understand the justification. Justification is mostly to align with the subsystem device. I like the '1-indexed' approach better. And the -1 causes Coverity to complain before the assert was added. > Assuming you hit a > bug and try to protect yourself, maybe now you should also > check for > > assert(n->namespaces[0] == NULL); > > somewhere. In nvme_ns() maybe? > That is definitely a state that should always hold, I guess we can do that, but we do already guard all "insertions" into the namespace array by an assert on the nsid. Then again, asserting here makes sure that we don't introduce something else that inserts on this invalid position. So, good point, I'll add it. > > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com> > > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com> > > --- > > hw/block/nvme.h | 8 ++++---- > > hw/block/nvme.c | 2 +- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/hw/block/nvme.h b/hw/block/nvme.h > > index 9edc86d79e98..c610ab30dc5c 100644 > > --- a/hw/block/nvme.h > > +++ b/hw/block/nvme.h > > @@ -217,7 +217,7 @@ typedef struct NvmeCtrl { > > * Attached namespaces to this controller. If subsys is not given, all > > * namespaces in this list will always be attached. > > */ > > - NvmeNamespace *namespaces[NVME_MAX_NAMESPACES]; > > + NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1]; > > NvmeSQueue **sq; > > NvmeCQueue **cq; > > NvmeSQueue admin_sq; > > @@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid) > > return NULL; > > } > > > > - return n->namespaces[nsid - 1]; > > + return n->namespaces[nsid]; > > } > > > > static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns) > > @@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns) > > uint32_t nsid = nvme_nsid(ns); > > assert(nsid && nsid <= NVME_MAX_NAMESPACES); > > > > - n->namespaces[nsid - 1] = ns; > > + n->namespaces[nsid] = ns; > > } > > > > static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) > > @@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) > > uint32_t nsid = nvme_nsid(ns); > > assert(nsid && nsid <= NVME_MAX_NAMESPACES); > > > > - n->namespaces[nsid - 1] = NULL; > > + n->namespaces[nsid] = NULL; > > } > > > > static inline NvmeCQueue *nvme_cq(NvmeRequest *req) > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c > > index c54ec3c9523c..6d31d5b17a0b 100644 > > --- a/hw/block/nvme.c > > +++ b/hw/block/nvme.c > > @@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) > > return -1; > > } > > } else { > > - if (n->namespaces[nsid - 1]) { > > + if (n->namespaces[nsid]) { > > error_setg(errp, "namespace id '%d' is already in use", nsid); > > return -1; > > } > > > >
On Apr 6 09:28, Klaus Jensen wrote: > On Apr 6 09:01, Philippe Mathieu-Daudé wrote: > > On 4/5/21 7:54 PM, Klaus Jensen wrote: > > > From: Klaus Jensen <k.jensen@samsung.com> > > > > > > The controller namespaces array being 0-indexed requires 'nsid - 1' > > > everywhere. Something that is easy to miss. Align the controller > > > namespaces array with the subsystem namespaces array such that both are > > > 1-indexed. > > > > TBH I don't understand the justification. > > Justification is mostly to align with the subsystem device. I like the > '1-indexed' approach better. And the -1 causes Coverity to complain > before the assert was added. > > > Assuming you hit a > > bug and try to protect yourself, maybe now you should also > > check for > > > > assert(n->namespaces[0] == NULL); > > > > somewhere. In nvme_ns() maybe? > > > > That is definitely a state that should always hold, I guess we can do > that, but we do already guard all "insertions" into the namespace array > by an assert on the nsid. Then again, asserting here makes sure that we > don't introduce something else that inserts on this invalid position. > > So, good point, I'll add it. > Then again again. I don't see the reason for the assert. Even if something ends up there by mistake we will never return it. If something ends up there due to new code, that nvme_ns() will always return NULL when nsid is zero and that should weed out the bug easily. I'll update the commit message to make it clear that this is about making both the subsystem and controller namespaces arrays 1-indexed. Them being indexed differently is a recipe for disaster I'd say. In anycase, I it actually a stretch to call this a bug fix, so I'll drop it and queue it up for v6.1.
diff --git a/hw/block/nvme.h b/hw/block/nvme.h index 9edc86d79e98..c610ab30dc5c 100644 --- a/hw/block/nvme.h +++ b/hw/block/nvme.h @@ -217,7 +217,7 @@ typedef struct NvmeCtrl { * Attached namespaces to this controller. If subsys is not given, all * namespaces in this list will always be attached. */ - NvmeNamespace *namespaces[NVME_MAX_NAMESPACES]; + NvmeNamespace *namespaces[NVME_MAX_NAMESPACES + 1]; NvmeSQueue **sq; NvmeCQueue **cq; NvmeSQueue admin_sq; @@ -232,7 +232,7 @@ static inline NvmeNamespace *nvme_ns(NvmeCtrl *n, uint32_t nsid) return NULL; } - return n->namespaces[nsid - 1]; + return n->namespaces[nsid]; } static inline bool nvme_ns_is_attached(NvmeCtrl *n, NvmeNamespace *ns) @@ -253,7 +253,7 @@ static inline void nvme_ns_attach(NvmeCtrl *n, NvmeNamespace *ns) uint32_t nsid = nvme_nsid(ns); assert(nsid && nsid <= NVME_MAX_NAMESPACES); - n->namespaces[nsid - 1] = ns; + n->namespaces[nsid] = ns; } static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) @@ -261,7 +261,7 @@ static inline void nvme_ns_detach(NvmeCtrl *n, NvmeNamespace *ns) uint32_t nsid = nvme_nsid(ns); assert(nsid && nsid <= NVME_MAX_NAMESPACES); - n->namespaces[nsid - 1] = NULL; + n->namespaces[nsid] = NULL; } static inline NvmeCQueue *nvme_cq(NvmeRequest *req) diff --git a/hw/block/nvme.c b/hw/block/nvme.c index c54ec3c9523c..6d31d5b17a0b 100644 --- a/hw/block/nvme.c +++ b/hw/block/nvme.c @@ -5915,7 +5915,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace *ns, Error **errp) return -1; } } else { - if (n->namespaces[nsid - 1]) { + if (n->namespaces[nsid]) { error_setg(errp, "namespace id '%d' is already in use", nsid); return -1; }