| Message ID | 20180718160135.11738-1-bart.vanassche@wdc.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
On Wed, Jul 18, 2018 at 09:01:35AM -0700, Bart Van Assche wrote: > This patch avoids that the following compiler warning is reported when > building with gcc 8 and W=1: > > In function 'ocrdma_mbx_get_ctrl_attribs', > inlined from 'ocrdma_init_hw' at drivers/infiniband/hw/ocrdma/ocrdma_hw.c:3224:11: > drivers/infiniband/hw/ocrdma/ocrdma_hw.c:1368:3: warning: 'strncpy' output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation] > strncpy(dev->model_number, > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > hba_attribs->controller_model_number, 31); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Selvin Xavier <selvin.xavier@broadcom.com> > Cc: Devesh Sharma <devesh.sharma@broadcom.com> > drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c > index c6c87cba943b..e578281471af 100644 > +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c > @@ -1365,8 +1365,9 @@ static int ocrdma_mbx_get_ctrl_attribs(struct ocrdma_dev *dev) > dev->hba_port_num = (hba_attribs->ptpnum_maxdoms_hbast_cv & > OCRDMA_HBA_ATTRB_PTNUM_MASK) > >> OCRDMA_HBA_ATTRB_PTNUM_SHIFT; > - strncpy(dev->model_number, > - hba_attribs->controller_model_number, 31); > + strlcpy(dev->model_number, > + hba_attribs->controller_model_number, > + sizeof(dev->model_number)); Yikes this code is weird. Why was it hard coded at 31 when both arrays are length 32? Selvin? Jason -- 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 Tue, Jul 24, 2018 at 3:03 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Wed, Jul 18, 2018 at 09:01:35AM -0700, Bart Van Assche wrote: >> This patch avoids that the following compiler warning is reported when >> building with gcc 8 and W=1: >> >> In function 'ocrdma_mbx_get_ctrl_attribs', >> inlined from 'ocrdma_init_hw' at drivers/infiniband/hw/ocrdma/ocrdma_hw.c:3224:11: >> drivers/infiniband/hw/ocrdma/ocrdma_hw.c:1368:3: warning: 'strncpy' output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation] >> strncpy(dev->model_number, >> ^~~~~~~~~~~~~~~~~~~~~~~~~~ >> hba_attribs->controller_model_number, 31); >> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> >> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> >> Cc: Selvin Xavier <selvin.xavier@broadcom.com> >> Cc: Devesh Sharma <devesh.sharma@broadcom.com> >> drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 +++-- >> 1 file changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c >> index c6c87cba943b..e578281471af 100644 >> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c >> @@ -1365,8 +1365,9 @@ static int ocrdma_mbx_get_ctrl_attribs(struct ocrdma_dev *dev) >> dev->hba_port_num = (hba_attribs->ptpnum_maxdoms_hbast_cv & >> OCRDMA_HBA_ATTRB_PTNUM_MASK) >> >> OCRDMA_HBA_ATTRB_PTNUM_SHIFT; >> - strncpy(dev->model_number, >> - hba_attribs->controller_model_number, 31); >> + strlcpy(dev->model_number, >> + hba_attribs->controller_model_number, >> + sizeof(dev->model_number)); > > Yikes this code is weird. Why was it hard coded at 31 when both arrays > are length 32? Selvin? > Seems like a bug. FW structure defines model_number as 32 byte array. But usually FW populates only 12 bytes out of 32 bytes for model_number. So it was never reported as an issue. Thank you for fixing it. Acked-by: Selvin Xavier <selvin.xavier@broadcom.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 Tue, Jul 24, 2018 at 2:40 AM, Selvin Xavier <selvin.xavier@broadcom.com> wrote: > On Tue, Jul 24, 2018 at 3:03 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >> On Wed, Jul 18, 2018 at 09:01:35AM -0700, Bart Van Assche wrote: >>> This patch avoids that the following compiler warning is reported when >>> building with gcc 8 and W=1: >>> >>> In function 'ocrdma_mbx_get_ctrl_attribs', >>> inlined from 'ocrdma_init_hw' at drivers/infiniband/hw/ocrdma/ocrdma_hw.c:3224:11: >>> drivers/infiniband/hw/ocrdma/ocrdma_hw.c:1368:3: warning: 'strncpy' output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation] >>> strncpy(dev->model_number, >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ >>> hba_attribs->controller_model_number, 31); >>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> >>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> >>> Cc: Selvin Xavier <selvin.xavier@broadcom.com> >>> Cc: Devesh Sharma <devesh.sharma@broadcom.com> >>> drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 +++-- >>> 1 file changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c >>> index c6c87cba943b..e578281471af 100644 >>> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c >>> @@ -1365,8 +1365,9 @@ static int ocrdma_mbx_get_ctrl_attribs(struct ocrdma_dev *dev) >>> dev->hba_port_num = (hba_attribs->ptpnum_maxdoms_hbast_cv & >>> OCRDMA_HBA_ATTRB_PTNUM_MASK) >>> >> OCRDMA_HBA_ATTRB_PTNUM_SHIFT; >>> - strncpy(dev->model_number, >>> - hba_attribs->controller_model_number, 31); >>> + strlcpy(dev->model_number, >>> + hba_attribs->controller_model_number, >>> + sizeof(dev->model_number)); >> >> Yikes this code is weird. Why was it hard coded at 31 when both arrays >> are length 32? Selvin? >> > Seems like a bug. FW structure defines model_number as 32 byte array. > But usually FW populates only 12 bytes out of 32 bytes for > model_number. So it was > never reported as an issue. So, the issue is the NULL termination, ideally we want to copy up to 32 bytes from the firmware into 33 bytes of destination array, so that there is no truncation and it is always null terminated. Unless you think we need to trust the firmware, then the strlcpy is fine. Jason -- 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, Jul 25, 2018 at 3:16 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > On Tue, Jul 24, 2018 at 2:40 AM, Selvin Xavier > <selvin.xavier@broadcom.com> wrote: >> On Tue, Jul 24, 2018 at 3:03 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >>> On Wed, Jul 18, 2018 at 09:01:35AM -0700, Bart Van Assche wrote: >>>> This patch avoids that the following compiler warning is reported when >>>> building with gcc 8 and W=1: >>>> >>>> In function 'ocrdma_mbx_get_ctrl_attribs', >>>> inlined from 'ocrdma_init_hw' at drivers/infiniband/hw/ocrdma/ocrdma_hw.c:3224:11: >>>> drivers/infiniband/hw/ocrdma/ocrdma_hw.c:1368:3: warning: 'strncpy' output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation] >>>> strncpy(dev->model_number, >>>> ^~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> hba_attribs->controller_model_number, 31); >>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>>> >>>> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> >>>> Cc: Selvin Xavier <selvin.xavier@broadcom.com> >>>> Cc: Devesh Sharma <devesh.sharma@broadcom.com> >>>> drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 +++-- >>>> 1 file changed, 3 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c >>>> index c6c87cba943b..e578281471af 100644 >>>> +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c >>>> @@ -1365,8 +1365,9 @@ static int ocrdma_mbx_get_ctrl_attribs(struct ocrdma_dev *dev) >>>> dev->hba_port_num = (hba_attribs->ptpnum_maxdoms_hbast_cv & >>>> OCRDMA_HBA_ATTRB_PTNUM_MASK) >>>> >> OCRDMA_HBA_ATTRB_PTNUM_SHIFT; >>>> - strncpy(dev->model_number, >>>> - hba_attribs->controller_model_number, 31); >>>> + strlcpy(dev->model_number, >>>> + hba_attribs->controller_model_number, >>>> + sizeof(dev->model_number)); >>> >>> Yikes this code is weird. Why was it hard coded at 31 when both arrays >>> are length 32? Selvin? >>> >> Seems like a bug. FW structure defines model_number as 32 byte array. >> But usually FW populates only 12 bytes out of 32 bytes for >> model_number. So it was >> never reported as an issue. > > So, the issue is the NULL termination, ideally we want to copy up to > 32 bytes from the firmware into 33 bytes of destination array, so that > there is no truncation and it is always null terminated. > > Unless you think we need to trust the firmware, then the strlcpy is fine. > We could trust FW. FW populates only 12 bytes and rest of the bytes are all NULL. But i still feel using strlcpy is better. That would prevent any scenarios in which Firmware decides to use all 32 bytes without a NULL termination. I dont think any ocrdma adapters available today uses all 32 bytes. If it is there, I think It is okay to truncate model number by one character. Thanks Selvin > Jason -- 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, Jul 18, 2018 at 09:01:35AM -0700, Bart Van Assche wrote: > This patch avoids that the following compiler warning is reported when > building with gcc 8 and W=1: > > In function 'ocrdma_mbx_get_ctrl_attribs', > inlined from 'ocrdma_init_hw' at drivers/infiniband/hw/ocrdma/ocrdma_hw.c:3224:11: > drivers/infiniband/hw/ocrdma/ocrdma_hw.c:1368:3: warning: 'strncpy' output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation] > strncpy(dev->model_number, > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > hba_attribs->controller_model_number, 31); > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> > Cc: Selvin Xavier <selvin.xavier@broadcom.com> > Cc: Devesh Sharma <devesh.sharma@broadcom.com> > Acked-by: Selvin Xavier <selvin.xavier@broadcom.com> > --- > drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Applied to for-next Thanks, Jason -- 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/hw/ocrdma/ocrdma_hw.c b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c index c6c87cba943b..e578281471af 100644 --- a/drivers/infiniband/hw/ocrdma/ocrdma_hw.c +++ b/drivers/infiniband/hw/ocrdma/ocrdma_hw.c @@ -1365,8 +1365,9 @@ static int ocrdma_mbx_get_ctrl_attribs(struct ocrdma_dev *dev) dev->hba_port_num = (hba_attribs->ptpnum_maxdoms_hbast_cv & OCRDMA_HBA_ATTRB_PTNUM_MASK) >> OCRDMA_HBA_ATTRB_PTNUM_SHIFT; - strncpy(dev->model_number, - hba_attribs->controller_model_number, 31); + strlcpy(dev->model_number, + hba_attribs->controller_model_number, + sizeof(dev->model_number)); } dma_free_coherent(&dev->nic_info.pdev->dev, dma.size, dma.va, dma.pa); free_mqe:
This patch avoids that the following compiler warning is reported when building with gcc 8 and W=1: In function 'ocrdma_mbx_get_ctrl_attribs', inlined from 'ocrdma_init_hw' at drivers/infiniband/hw/ocrdma/ocrdma_hw.c:3224:11: drivers/infiniband/hw/ocrdma/ocrdma_hw.c:1368:3: warning: 'strncpy' output may be truncated copying 31 bytes from a string of length 31 [-Wstringop-truncation] strncpy(dev->model_number, ^~~~~~~~~~~~~~~~~~~~~~~~~~ hba_attribs->controller_model_number, 31); ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com> Cc: Selvin Xavier <selvin.xavier@broadcom.com> Cc: Devesh Sharma <devesh.sharma@broadcom.com> --- drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)