This fixes device lifetime issues where it was possible to free a live
struct device.
Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
1 file changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
index 8f84a48508ac..47a6a9dfc9e8 100644
--- a/drivers/counter/intel-qep.c
+++ b/drivers/counter/intel-qep.c
@@ -63,7 +63,6 @@
#define INTEL_QEP_CLK_PERIOD_NS 10
struct intel_qep {
- struct counter_device counter;
struct mutex lock;
struct device *dev;
void __iomem *regs;
@@ -392,14 +391,16 @@ static struct counter_count intel_qep_counter_count[] = {
static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
{
+ struct counter_device *counter;
struct intel_qep *qep;
struct device *dev = &pci->dev;
void __iomem *regs;
int ret;
- qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
- if (!qep)
+ counter = devm_counter_alloc(dev, sizeof(*qep));
+ if (!counter)
return -ENOMEM;
+ qep = counter_priv(counter);
ret = pcim_enable_device(pci);
if (ret)
@@ -422,20 +423,23 @@ static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
intel_qep_init(qep);
pci_set_drvdata(pci, qep);
- qep->counter.name = pci_name(pci);
- qep->counter.parent = dev;
- qep->counter.ops = &intel_qep_counter_ops;
- qep->counter.counts = intel_qep_counter_count;
- qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
- qep->counter.signals = intel_qep_signals;
- qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
- qep->counter.priv = qep;
+ counter->name = pci_name(pci);
+ counter->parent = dev;
+ counter->ops = &intel_qep_counter_ops;
+ counter->counts = intel_qep_counter_count;
+ counter->num_counts = ARRAY_SIZE(intel_qep_counter_count);
+ counter->signals = intel_qep_signals;
+ counter->num_signals = ARRAY_SIZE(intel_qep_signals);
qep->enabled = false;
pm_runtime_put(dev);
pm_runtime_allow(dev);
- return devm_counter_register(&pci->dev, &qep->counter);
+ ret = devm_counter_add(&pci->dev, counter);
+ if (ret < 0)
+ return dev_err_probe(&pci->dev, ret, "Failed to add counter\n");
+
+ return 0;
}
static void intel_qep_remove(struct pci_dev *pci)
--
2.33.0
On 12/27/21 11:45, Uwe Kleine-König wrote:
> This fixes device lifetime issues where it was possible to free a live
> struct device.
>
> Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
Should the Fixes tag rather be b6c50affda59 ("counter: Add character
device interface") instead of when each drivers were introduced? I mean
was it possible to hit the issue before /dev/counter was introduced?
Jarkko
On Mon, Dec 27, 2021 at 05:02:39PM +0200, Jarkko Nikula wrote:
> On 12/27/21 11:45, Uwe Kleine-K?nig wrote:
> > This fixes device lifetime issues where it was possible to free a live
> > struct device.
> >
> > Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
> > Signed-off-by: Uwe Kleine-K?nig <[email protected]>
> > ---
> > drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
> > 1 file changed, 16 insertions(+), 12 deletions(-)
> >
> Should the Fixes tag rather be b6c50affda59 ("counter: Add character device
> interface") instead of when each drivers were introduced? I mean was it
> possible to hit the issue before /dev/counter was introduced?
I'm not sure if there is an issue before this, but it was already wrong
before for sure. Maybe it's possible to hold a reference somehow via
sysfs?
The thought that made me tag the individual driver commits was: With the
approach used to fix the issue all drivers need to be modified and an
unconverted driver doesn't benefit from the availability of
counter_alloc() / counter_add() if it isn't used. So all trees that
include b711f687a1c1 but not "counter: intel-qep: Convert to new counter
registration" are broken (more or less exploitable). So I think the
added Fixes line is the right choice.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-K?nig |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, 27 Dec 2021 10:45:20 +0100
Uwe Kleine-König <[email protected]> wrote:
> This fixes device lifetime issues where it was possible to free a live
> struct device.
>
> Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
> Signed-off-by: Uwe Kleine-König <[email protected]>
FWIW given these are all more or less the same...
Reviewed-by: Jonathan Cameron <[email protected]>
> ---
> drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> index 8f84a48508ac..47a6a9dfc9e8 100644
> --- a/drivers/counter/intel-qep.c
> +++ b/drivers/counter/intel-qep.c
> @@ -63,7 +63,6 @@
> #define INTEL_QEP_CLK_PERIOD_NS 10
>
> struct intel_qep {
> - struct counter_device counter;
> struct mutex lock;
> struct device *dev;
> void __iomem *regs;
> @@ -392,14 +391,16 @@ static struct counter_count intel_qep_counter_count[] = {
>
> static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> {
> + struct counter_device *counter;
> struct intel_qep *qep;
> struct device *dev = &pci->dev;
> void __iomem *regs;
> int ret;
>
> - qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
> - if (!qep)
> + counter = devm_counter_alloc(dev, sizeof(*qep));
> + if (!counter)
> return -ENOMEM;
> + qep = counter_priv(counter);
>
> ret = pcim_enable_device(pci);
> if (ret)
> @@ -422,20 +423,23 @@ static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> intel_qep_init(qep);
> pci_set_drvdata(pci, qep);
>
> - qep->counter.name = pci_name(pci);
> - qep->counter.parent = dev;
> - qep->counter.ops = &intel_qep_counter_ops;
> - qep->counter.counts = intel_qep_counter_count;
> - qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
> - qep->counter.signals = intel_qep_signals;
> - qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
> - qep->counter.priv = qep;
> + counter->name = pci_name(pci);
> + counter->parent = dev;
> + counter->ops = &intel_qep_counter_ops;
> + counter->counts = intel_qep_counter_count;
> + counter->num_counts = ARRAY_SIZE(intel_qep_counter_count);
> + counter->signals = intel_qep_signals;
> + counter->num_signals = ARRAY_SIZE(intel_qep_signals);
> qep->enabled = false;
>
> pm_runtime_put(dev);
> pm_runtime_allow(dev);
>
> - return devm_counter_register(&pci->dev, &qep->counter);
> + ret = devm_counter_add(&pci->dev, counter);
> + if (ret < 0)
> + return dev_err_probe(&pci->dev, ret, "Failed to add counter\n");
> +
> + return 0;
> }
>
> static void intel_qep_remove(struct pci_dev *pci)
On Mon, Dec 27, 2021 at 10:45:20AM +0100, Uwe Kleine-König wrote:
> This fixes device lifetime issues where it was possible to free a live
> struct device.
>
> Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
> Signed-off-by: Uwe Kleine-König <[email protected]>
Acked-by: William Breathitt Gray <[email protected]>
> ---
> drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
> 1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> index 8f84a48508ac..47a6a9dfc9e8 100644
> --- a/drivers/counter/intel-qep.c
> +++ b/drivers/counter/intel-qep.c
> @@ -63,7 +63,6 @@
> #define INTEL_QEP_CLK_PERIOD_NS 10
>
> struct intel_qep {
> - struct counter_device counter;
> struct mutex lock;
> struct device *dev;
> void __iomem *regs;
> @@ -392,14 +391,16 @@ static struct counter_count intel_qep_counter_count[] = {
>
> static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> {
> + struct counter_device *counter;
> struct intel_qep *qep;
> struct device *dev = &pci->dev;
> void __iomem *regs;
> int ret;
>
> - qep = devm_kzalloc(dev, sizeof(*qep), GFP_KERNEL);
> - if (!qep)
> + counter = devm_counter_alloc(dev, sizeof(*qep));
> + if (!counter)
> return -ENOMEM;
> + qep = counter_priv(counter);
>
> ret = pcim_enable_device(pci);
> if (ret)
> @@ -422,20 +423,23 @@ static int intel_qep_probe(struct pci_dev *pci, const struct pci_device_id *id)
> intel_qep_init(qep);
> pci_set_drvdata(pci, qep);
>
> - qep->counter.name = pci_name(pci);
> - qep->counter.parent = dev;
> - qep->counter.ops = &intel_qep_counter_ops;
> - qep->counter.counts = intel_qep_counter_count;
> - qep->counter.num_counts = ARRAY_SIZE(intel_qep_counter_count);
> - qep->counter.signals = intel_qep_signals;
> - qep->counter.num_signals = ARRAY_SIZE(intel_qep_signals);
> - qep->counter.priv = qep;
> + counter->name = pci_name(pci);
> + counter->parent = dev;
> + counter->ops = &intel_qep_counter_ops;
> + counter->counts = intel_qep_counter_count;
> + counter->num_counts = ARRAY_SIZE(intel_qep_counter_count);
> + counter->signals = intel_qep_signals;
> + counter->num_signals = ARRAY_SIZE(intel_qep_signals);
> qep->enabled = false;
>
> pm_runtime_put(dev);
> pm_runtime_allow(dev);
>
> - return devm_counter_register(&pci->dev, &qep->counter);
> + ret = devm_counter_add(&pci->dev, counter);
> + if (ret < 0)
> + return dev_err_probe(&pci->dev, ret, "Failed to add counter\n");
> +
> + return 0;
> }
>
> static void intel_qep_remove(struct pci_dev *pci)
> --
> 2.33.0
>
On 12/28/21 12:56, Uwe Kleine-König wrote:
> On Mon, Dec 27, 2021 at 05:02:39PM +0200, Jarkko Nikula wrote:
>> On 12/27/21 11:45, Uwe Kleine-König wrote:
>>> This fixes device lifetime issues where it was possible to free a live
>>> struct device.
>>>
>>> Fixes: b711f687a1c1 ("counter: Add support for Intel Quadrature Encoder Peripheral")
>>> Signed-off-by: Uwe Kleine-König <[email protected]>
>>> ---
>>> drivers/counter/intel-qep.c | 28 ++++++++++++++++------------
>>> 1 file changed, 16 insertions(+), 12 deletions(-)
>>>
>> Should the Fixes tag rather be b6c50affda59 ("counter: Add character device
>> interface") instead of when each drivers were introduced? I mean was it
>> possible to hit the issue before /dev/counter was introduced?
>
> I'm not sure if there is an issue before this, but it was already wrong
> before for sure. Maybe it's possible to hold a reference somehow via
> sysfs?
>
> The thought that made me tag the individual driver commits was: With the
> approach used to fix the issue all drivers need to be modified and an
> unconverted driver doesn't benefit from the availability of
> counter_alloc() / counter_add() if it isn't used. So all trees that
> include b711f687a1c1 but not "counter: intel-qep: Convert to new counter
> registration" are broken (more or less exploitable). So I think the
> added Fixes line is the right choice.
>
Fair enough. Noticed only now after this patch (with patches 1, 4, 8 and
13 applied) second counter instance initialization fails:
[ 8.113999] sysfs: cannot create duplicate filename '/dev/char/244:0
[ 8.139604] Call Trace:
[ 8.139606] <TASK>
[ 8.139608] show_stack+0x3d/0x3f
[ 8.139615] dump_stack_lvl+0x5b/0x82
[ 8.139619] dump_stack+0x10/0x12
[ 8.139621] sysfs_warn_dup.cold+0x17/0x27
[ 8.139624] sysfs_do_create_link_sd+0xc2/0xd0
[ 8.139629] sysfs_create_link+0x1c/0x30
[ 8.139631] device_add+0x54f/0x7c0
[ 8.139635] ? cdev_default_release+0x20/0x20
[ 8.139638] cdev_device_add+0x47/0x90
[ 8.139642] devm_counter_add+0x61/0xe0 [counter]
[ 8.139647] intel_qep_probe+0x16f/0x1b0
...
[ 8.325081] intel-qep 0000:00:18.6: error -EEXIST: Failed to add counter
[ 8.371793] intel-qep: probe of 0000:00:18.6 failed with error -17
Positive result is that following test doesn't produce an oops after a
few iterations like before this patch.
while :; do { sleep 5; echo bang; } > /dev/counter0 & sleep 1; echo
0000:00:18.4 >/sys/bus/pci/drivers/intel-qep/unbind; sleep 8; echo
0000:00:18.4 >/sys/bus/pci/drivers/intel-qep/bind; done