| Message ID | 20210203135936.23016-1-colin.king@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [next] iommu/mediatek: Fix unsigned domid comparison with less than zero | expand |
On Wed, 2021-02-03 at 13:59 +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently the check for domid < 0 is always false because domid > is unsigned. Fix this by making it signed. > > Addresses-CoverityL ("Unsigned comparison against 0") > Fixes: ab1d5281a62b ("iommu/mediatek: Add iova reserved function") > Signed-off-by: Colin Ian King <colin.king@canonical.com> Thanks for the fix. Reviewed-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 0ad14a7604b1..823d719945b2 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -640,7 +640,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev, > struct list_head *head) > { > struct mtk_iommu_data *data = dev_iommu_priv_get(dev); > - unsigned int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > + int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > const struct mtk_iommu_iova_region *resv, *curdom; > struct iommu_resv_region *region; > int prot = IOMMU_WRITE | IOMMU_READ;
On Wed, Feb 03, 2021 at 01:59:36PM +0000, Colin King wrote: > From: Colin Ian King <colin.king@canonical.com> > > Currently the check for domid < 0 is always false because domid > is unsigned. Fix this by making it signed. > > Addresses-CoverityL ("Unsigned comparison against 0") Typo here ('L' instead of ':') > Fixes: ab1d5281a62b ("iommu/mediatek: Add iova reserved function") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > drivers/iommu/mtk_iommu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 0ad14a7604b1..823d719945b2 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -640,7 +640,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev, > struct list_head *head) > { > struct mtk_iommu_data *data = dev_iommu_priv_get(dev); > - unsigned int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > + int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; Not sure if it's intentional, but this also makes 'i' signed. It probably should remain 'unsigned' to match 'iova_region_nr' in 'struct mtk_iommu_plat_data'. Will
On Thu, Feb 04, 2021 at 09:25:58AM +0000, Will Deacon wrote: > On Wed, Feb 03, 2021 at 01:59:36PM +0000, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > Currently the check for domid < 0 is always false because domid > > is unsigned. Fix this by making it signed. > > > > Addresses-CoverityL ("Unsigned comparison against 0") > > Typo here ('L' instead of ':') > > > Fixes: ab1d5281a62b ("iommu/mediatek: Add iova reserved function") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/iommu/mtk_iommu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 0ad14a7604b1..823d719945b2 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -640,7 +640,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev, > > struct list_head *head) > > { > > struct mtk_iommu_data *data = dev_iommu_priv_get(dev); > > - unsigned int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > > + int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > > Not sure if it's intentional, but this also makes 'i' signed. It probably > should remain 'unsigned' to match 'iova_region_nr' in > 'struct mtk_iommu_plat_data'. Yes, 'i' should stay unsigned. Colin, can you please fix that up and re-send? Thanks, Joerg
On Thu, Feb 04, 2021 at 09:25:58AM +0000, Will Deacon wrote: > On Wed, Feb 03, 2021 at 01:59:36PM +0000, Colin King wrote: > > From: Colin Ian King <colin.king@canonical.com> > > > > Currently the check for domid < 0 is always false because domid > > is unsigned. Fix this by making it signed. > > > > Addresses-CoverityL ("Unsigned comparison against 0") > > Typo here ('L' instead of ':') > > > Fixes: ab1d5281a62b ("iommu/mediatek: Add iova reserved function") > > Signed-off-by: Colin Ian King <colin.king@canonical.com> > > --- > > drivers/iommu/mtk_iommu.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index 0ad14a7604b1..823d719945b2 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -640,7 +640,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev, > > struct list_head *head) > > { > > struct mtk_iommu_data *data = dev_iommu_priv_get(dev); > > - unsigned int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > > + int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; > > Not sure if it's intentional, but this also makes 'i' signed. It probably > should remain 'unsigned' to match 'iova_region_nr' in > 'struct mtk_iommu_plat_data'. iova_region_nr is either 1 or 5 so unsigned doesn't matter. I once almost introduced a bug where the iterator was supposed to be size_t. I fixed a bug by making it signed but I ended up introducing a new bug. But generally that's pretty rare. The more common case is that making iterators unsigned introduces bugs. It's better to default to "int i;" and if more complicated types are required that should stand out. "size_t pg_idx;" or whatever. regards, dan carpenter
I second that ... Having i unsigned violates the rule of "least surprise". If you need it unsigned make it clearly visible, also adding a simple comment may help. jm2c, wh
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c index 0ad14a7604b1..823d719945b2 100644 --- a/drivers/iommu/mtk_iommu.c +++ b/drivers/iommu/mtk_iommu.c @@ -640,7 +640,7 @@ static void mtk_iommu_get_resv_regions(struct device *dev, struct list_head *head) { struct mtk_iommu_data *data = dev_iommu_priv_get(dev); - unsigned int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; + int domid = mtk_iommu_get_domain_id(dev, data->plat_data), i; const struct mtk_iommu_iova_region *resv, *curdom; struct iommu_resv_region *region; int prot = IOMMU_WRITE | IOMMU_READ;