On Thu, Apr 14, 2022 at 03:00:30PM +0800, cam enih wrote:
> sorry for confusion.
Again, please do not top-post.
> 1. MR=merge request;
We do not have merge requests in the kernel, so I do not understand what
you are saying we do differently here.
> 2. In this case, the change depends on a non-existing config option which I
> believe can be detected at compile time.
Specifically, please show us how we should have caught this so we can do
that in the future. I do not see how this can be detected at compile
time as it is no different from a normal config option not being
selected.
thanks,
greg k-h
A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
On Thu, Apr 14, 2022 at 01:50:05PM +0800, cam enih wrote:
> thanks everyone.
>
> two suggestions:
>
> 1. To avoid this issue from future dot builds of LTS, i would suggest to
> avoid MRs of any config/module dependency changes, unless the scope of
> impact limits to the scope of MR itself.
What is a "mr"?
> 2. this issue can fail-fast by checking the dependency in compile time.
And how could we have tested this?
confused,
greg k-h
The bug is here:
if (!iommu || iommu->dev->of_node != spec->np) {
The list iterator value 'iommu' will *always* be set and non-NULL by
list_for_each_entry(), so it is incorrect to assume that the iterator
value will be NULL if the list is empty or no element is found (in fact,
it will point to a invalid structure object containing HEAD).
To fix the bug, run insert_iommu_master(dev, &iommu, spec); unlock and
return 0 when found, otherwise unlock and return -ENODEV.
Cc: stable(a)vger.kernel.org
Fixes: f78ebca8ff3d6 ("iommu/msm: Add support for generic master bindings")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong(a)gmail.com>
---
drivers/iommu/msm_iommu.c | 18 ++++++------------
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/iommu/msm_iommu.c b/drivers/iommu/msm_iommu.c
index 3a38352b603f..1dbb8b0695ec 100644
--- a/drivers/iommu/msm_iommu.c
+++ b/drivers/iommu/msm_iommu.c
@@ -617,23 +617,17 @@ static int qcom_iommu_of_xlate(struct device *dev,
{
struct msm_iommu_dev *iommu;
unsigned long flags;
- int ret = 0;
spin_lock_irqsave(&msm_iommu_lock, flags);
list_for_each_entry(iommu, &qcom_iommu_devices, dev_node)
- if (iommu->dev->of_node == spec->np)
- break;
-
- if (!iommu || iommu->dev->of_node != spec->np) {
- ret = -ENODEV;
- goto fail;
- }
-
- insert_iommu_master(dev, &iommu, spec);
-fail:
+ if (iommu->dev->of_node == spec->np) {
+ insert_iommu_master(dev, &iommu, spec);
+ spin_unlock_irqrestore(&msm_iommu_lock, flags);
+ return 0;
+ }
spin_unlock_irqrestore(&msm_iommu_lock, flags);
- return ret;
+ return -ENODEV;
}
irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
--
2.17.1
The bug is here:
if (SCB_out == scb->phys)
The list iterator 'scb' will point to a bogus position containing
HEAD if the list is empty or no element is found. This case must
be checked before any use of the iterator, otherwise it will lead
to a invalid memory access.
To fix this bug, add an check. Use a new variable 'iter' as the
list iterator, while use the old variable 'scb' as a dedicated
pointer to point to the found element.
Cc: stable(a)vger.kernel.org
Fixes: 48a3103006631 ("wd719x: Introduce Western Digital WD7193/7197/7296 PCI SCSI card driver")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong(a)gmail.com>
---
drivers/scsi/wd719x.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/scsi/wd719x.c b/drivers/scsi/wd719x.c
index 1a7947554581..6087ff4c05da 100644
--- a/drivers/scsi/wd719x.c
+++ b/drivers/scsi/wd719x.c
@@ -684,11 +684,15 @@ static irqreturn_t wd719x_interrupt(int irq, void *dev_id)
case WD719X_INT_SPIDERFAILED:
/* was the cmd completed a direct or SCB command? */
if (regs.bytes.OPC == WD719X_CMD_PROCESS_SCB) {
- struct wd719x_scb *scb;
- list_for_each_entry(scb, &wd->active_scbs, list)
- if (SCB_out == scb->phys)
+ struct wd719x_scb *scb = NULL, *iter;
+
+ list_for_each_entry(iter, &wd->active_scbs, list)
+ if (SCB_out == iter->phys) {
+ scb = iter;
break;
- if (SCB_out == scb->phys)
+ }
+
+ if (scb)
wd719x_interrupt_SCB(wd, regs, scb);
else
dev_err(&wd->pdev->dev, "card returned invalid SCB pointer\n");
--
2.17.1
The three bugs are here:
__func__, s3a_buf->s3a_data->exp_id);
__func__, md_buf->metadata->exp_id);
__func__, dis_buf->dis_data->exp_id);
The list iterator 's3a_buf/md_buf/dis_buf' will point to a bogus
position containing HEAD if the list is empty or no element is found.
This case must be checked before any use of the iterator, otherwise
it will lead to a invalid memory access.
To fix this bug, add an check. Use a new variable '*_iter' as the
list iterator, while use the old variable '*_buf' as a dedicated
pointer to point to the found element.
Cc: stable(a)vger.kernel.org
Fixes: ad85094b293e4 ("Revert "media: staging: atomisp: Remove driver"")
Signed-off-by: Xiaomeng Tong <xiam0nd.tong(a)gmail.com>
---
.../staging/media/atomisp/pci/atomisp_cmd.c | 57 ++++++++++++-------
1 file changed, 36 insertions(+), 21 deletions(-)
diff --git a/drivers/staging/media/atomisp/pci/atomisp_cmd.c b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
index 97d5a528969b..0da0b69a4637 100644
--- a/drivers/staging/media/atomisp/pci/atomisp_cmd.c
+++ b/drivers/staging/media/atomisp/pci/atomisp_cmd.c
@@ -901,9 +901,9 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
int err;
unsigned long irqflags;
struct ia_css_frame *frame = NULL;
- struct atomisp_s3a_buf *s3a_buf = NULL, *_s3a_buf_tmp;
- struct atomisp_dis_buf *dis_buf = NULL, *_dis_buf_tmp;
- struct atomisp_metadata_buf *md_buf = NULL, *_md_buf_tmp;
+ struct atomisp_s3a_buf *s3a_buf = NULL, *_s3a_buf_tmp, *s3a_iter;
+ struct atomisp_dis_buf *dis_buf = NULL, *_dis_buf_tmp, *dis_iter;
+ struct atomisp_metadata_buf *md_buf = NULL, *_md_buf_tmp, *md_iter;
enum atomisp_metadata_type md_type;
struct atomisp_device *isp = asd->isp;
struct v4l2_control ctrl;
@@ -942,60 +942,75 @@ void atomisp_buf_done(struct atomisp_sub_device *asd, int error,
switch (buf_type) {
case IA_CSS_BUFFER_TYPE_3A_STATISTICS:
- list_for_each_entry_safe(s3a_buf, _s3a_buf_tmp,
+ list_for_each_entry_safe(s3a_iter, _s3a_buf_tmp,
&asd->s3a_stats_in_css, list) {
- if (s3a_buf->s3a_data ==
+ if (s3a_iter->s3a_data ==
buffer.css_buffer.data.stats_3a) {
- list_del_init(&s3a_buf->list);
- list_add_tail(&s3a_buf->list,
+ list_del_init(&s3a_iter->list);
+ list_add_tail(&s3a_iter->list,
&asd->s3a_stats_ready);
+ s3a_buf = s3a_iter;
break;
}
}
asd->s3a_bufs_in_css[css_pipe_id]--;
atomisp_3a_stats_ready_event(asd, buffer.css_buffer.exp_id);
- dev_dbg(isp->dev, "%s: s3a stat with exp_id %d is ready\n",
- __func__, s3a_buf->s3a_data->exp_id);
+ if (s3a_buf)
+ dev_dbg(isp->dev, "%s: s3a stat with exp_id %d is ready\n",
+ __func__, s3a_buf->s3a_data->exp_id);
+ else
+ dev_dbg(isp->dev, "%s: s3a stat is ready with no exp_id found\n",
+ __func__);
break;
case IA_CSS_BUFFER_TYPE_METADATA:
if (error)
break;
md_type = atomisp_get_metadata_type(asd, css_pipe_id);
- list_for_each_entry_safe(md_buf, _md_buf_tmp,
+ list_for_each_entry_safe(md_iter, _md_buf_tmp,
&asd->metadata_in_css[md_type], list) {
- if (md_buf->metadata ==
+ if (md_iter->metadata ==
buffer.css_buffer.data.metadata) {
- list_del_init(&md_buf->list);
- list_add_tail(&md_buf->list,
+ list_del_init(&md_iter->list);
+ list_add_tail(&md_iter->list,
&asd->metadata_ready[md_type]);
+ md_buf = md_iter;
break;
}
}
asd->metadata_bufs_in_css[stream_id][css_pipe_id]--;
atomisp_metadata_ready_event(asd, md_type);
- dev_dbg(isp->dev, "%s: metadata with exp_id %d is ready\n",
- __func__, md_buf->metadata->exp_id);
+ if (md_buf)
+ dev_dbg(isp->dev, "%s: metadata with exp_id %d is ready\n",
+ __func__, md_buf->metadata->exp_id);
+ else
+ dev_dbg(isp->dev, "%s: metadata is ready with no exp_id found\n",
+ __func__);
break;
case IA_CSS_BUFFER_TYPE_DIS_STATISTICS:
- list_for_each_entry_safe(dis_buf, _dis_buf_tmp,
+ list_for_each_entry_safe(dis_iter, _dis_buf_tmp,
&asd->dis_stats_in_css, list) {
- if (dis_buf->dis_data ==
+ if (dis_iter->dis_data ==
buffer.css_buffer.data.stats_dvs) {
spin_lock_irqsave(&asd->dis_stats_lock,
irqflags);
- list_del_init(&dis_buf->list);
- list_add(&dis_buf->list, &asd->dis_stats);
+ list_del_init(&dis_iter->list);
+ list_add(&dis_iter->list, &asd->dis_stats);
asd->params.dis_proj_data_valid = true;
spin_unlock_irqrestore(&asd->dis_stats_lock,
irqflags);
+ dis_buf = dis_iter;
break;
}
}
asd->dis_bufs_in_css--;
- dev_dbg(isp->dev, "%s: dis stat with exp_id %d is ready\n",
- __func__, dis_buf->dis_data->exp_id);
+ if (dis_buf)
+ dev_dbg(isp->dev, "%s: dis stat with exp_id %d is ready\n",
+ __func__, dis_buf->dis_data->exp_id);
+ else
+ dev_dbg(isp->dev, "%s: dis stat is ready with no exp_id found\n",
+ __func__);
break;
case IA_CSS_BUFFER_TYPE_VF_OUTPUT_FRAME:
case IA_CSS_BUFFER_TYPE_SEC_VF_OUTPUT_FRAME:
--
2.17.1