| Message ID | 20180913151716.6333-4-wsa+renesas@sang-engineering.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-mapping: clear dangling pointers consistently | expand |
Same here. I don't even think we really need to clear the ops to start with, but we if we want to do it I'd just do it directly in the two callers.
On 13/09/18 16:17, Wolfram Sang wrote: > While sanitizig the pointer for dma_ops on teardown, do the same for > dma_parms, too. Rename the function to have a more generic name. Upon closer inspection, it looks like there are some cases (at least PCI and MFD) where dma_parms is installed by the parent/bus at device creation, and therefore remains valid and *would* be expected to persist across the child device's driver unbinding and rebinding - seems this is more complex than I first thought, sorry. Robin. > Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> > --- > arch/arm/mm/dma-mapping.c | 2 +- > include/linux/dma-mapping.h | 5 +++-- > 2 files changed, 4 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 466b0242e8af..bcf77bc0423f 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -2399,5 +2399,5 @@ void arch_teardown_dma_ops(struct device *dev) > if (dev->archdata.dma_ops_setup) > arm_teardown_iommu_dma_ops(dev); > > - generic_teardown_dma_ops(dev); > + generic_teardown_dma(dev); > } > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h > index 020512cb7f0e..6a2d8779b1d8 100644 > --- a/include/linux/dma-mapping.h > +++ b/include/linux/dma-mapping.h > @@ -663,12 +663,13 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, > bool coherent) { } > #endif > > -static inline void generic_teardown_dma_ops(struct device *dev) > +static inline void generic_teardown_dma(struct device *dev) > { > dev->dma_ops = NULL; > + dev->dma_parms = NULL; > } > #ifndef arch_teardown_dma_ops > -#define arch_teardown_dma_ops generic_teardown_dma_ops > +#define arch_teardown_dma_ops generic_teardown_dma > #endif > > static inline unsigned int dma_get_max_seg_size(struct device *dev) >
On Fri, Sep 14, 2018 at 02:23:51PM +0100, Robin Murphy wrote: > On 13/09/18 16:17, Wolfram Sang wrote: > > While sanitizig the pointer for dma_ops on teardown, do the same for > > dma_parms, too. Rename the function to have a more generic name. > > Upon closer inspection, it looks like there are some cases (at least PCI and > MFD) where dma_parms is installed by the parent/bus at device creation, and > therefore remains valid and *would* be expected to persist across the child > device's driver unbinding and rebinding - seems this is more complex than I > first thought, sorry. No problem. I am thankful I understand more details. So, the life cycle of dma_parms is truly that of the device. Which means that the drivers using devm_kzalloc in their probe, should ideally clear the pointer on unbind. Otherwise, it is not possible to say if the non-NULL dma_parms is intended or dangling. Which makes my initial dmam_set_dma_parms() approach look not too bad, I'd think? However, I don't want to push hard with this one. If you think this is too academic, I'll just leave it for now...
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index 466b0242e8af..bcf77bc0423f 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -2399,5 +2399,5 @@ void arch_teardown_dma_ops(struct device *dev) if (dev->archdata.dma_ops_setup) arm_teardown_iommu_dma_ops(dev); - generic_teardown_dma_ops(dev); + generic_teardown_dma(dev); } diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index 020512cb7f0e..6a2d8779b1d8 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -663,12 +663,13 @@ static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base, bool coherent) { } #endif -static inline void generic_teardown_dma_ops(struct device *dev) +static inline void generic_teardown_dma(struct device *dev) { dev->dma_ops = NULL; + dev->dma_parms = NULL; } #ifndef arch_teardown_dma_ops -#define arch_teardown_dma_ops generic_teardown_dma_ops +#define arch_teardown_dma_ops generic_teardown_dma #endif static inline unsigned int dma_get_max_seg_size(struct device *dev)
While sanitizig the pointer for dma_ops on teardown, do the same for dma_parms, too. Rename the function to have a more generic name. Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com> --- arch/arm/mm/dma-mapping.c | 2 +- include/linux/dma-mapping.h | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-)