* [RFT][PATCH 0/7] Avoid overflow at boundary_size
@ 2020-08-20 23:19 Nicolin Chen
2020-08-25 10:16 ` Niklas Schnelle
0 siblings, 1 reply; 3+ messages in thread
From: Nicolin Chen @ 2020-08-20 23:19 UTC (permalink / raw)
To: mpe, benh, paulus, rth, ink, mattst88, tony.luck, fenghua.yu,
schnelle, gerald.schaefer, hca, gor, borntraeger, davem, tglx,
mingo, bp, x86, hpa, James.Bottomley, deller
Cc: sfr, hch, linuxppc-dev, linux-kernel, linux-alpha, linux-ia64,
linux-s390, sparclinux, linux-parisc
We are expending the default DMA segmentation boundary to its
possible maximum value (ULONG_MAX) to indicate that a device
doesn't specify a boundary limit. So all dma_get_seg_boundary
callers should take a precaution with the return values since
it would easily get overflowed.
I scanned the entire kernel tree for all the existing callers
and found that most of callers may get overflowed in two ways:
either "+ 1" or passing it to ALIGN() that does "+ mask".
According to kernel defines:
#define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
#define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
We can simplify the logic here:
ALIGN(boundary + 1, 1 << shift) >> shift
= ALIGN_MASK(b + 1, (1 << s) - 1) >> s
= {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
= [b + 1 + (1 << s) - 1] >> s
= [b + (1 << s)] >> s
= (b >> s) + 1
So this series of patches fix the potential overflow with this
overflow-free shortcut.
As I don't think that I have these platforms, marking RFT.
Thanks
Nic
Nicolin Chen (7):
powerpc/iommu: Avoid overflow at boundary_size
alpha: Avoid overflow at boundary_size
ia64/sba_iommu: Avoid overflow at boundary_size
s390/pci_dma: Avoid overflow at boundary_size
sparc: Avoid overflow at boundary_size
x86/amd_gart: Avoid overflow at boundary_size
parisc: Avoid overflow at boundary_size
arch/alpha/kernel/pci_iommu.c | 10 ++++------
arch/ia64/hp/common/sba_iommu.c | 4 ++--
arch/powerpc/kernel/iommu.c | 11 +++++------
arch/s390/pci/pci_dma.c | 4 ++--
arch/sparc/kernel/iommu-common.c | 9 +++------
arch/sparc/kernel/iommu.c | 4 ++--
arch/sparc/kernel/pci_sun4v.c | 4 ++--
arch/x86/kernel/amd_gart_64.c | 4 ++--
drivers/parisc/ccio-dma.c | 4 ++--
drivers/parisc/sba_iommu.c | 4 ++--
10 files changed, 26 insertions(+), 32 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFT][PATCH 0/7] Avoid overflow at boundary_size
2020-08-20 23:19 [RFT][PATCH 0/7] Avoid overflow at boundary_size Nicolin Chen
@ 2020-08-25 10:16 ` Niklas Schnelle
2020-08-25 23:19 ` Nicolin Chen
0 siblings, 1 reply; 3+ messages in thread
From: Niklas Schnelle @ 2020-08-25 10:16 UTC (permalink / raw)
To: Nicolin Chen, mpe, benh, paulus, rth, ink, mattst88, tony.luck,
fenghua.yu, gerald.schaefer, hca, gor, borntraeger, davem, tglx,
mingo, bp, x86, hpa, James.Bottomley, deller
Cc: sfr, hch, linuxppc-dev, linux-kernel, linux-alpha, linux-ia64,
linux-s390, sparclinux, linux-parisc
On 8/21/20 1:19 AM, Nicolin Chen wrote:
> We are expending the default DMA segmentation boundary to its
> possible maximum value (ULONG_MAX) to indicate that a device
> doesn't specify a boundary limit. So all dma_get_seg_boundary
> callers should take a precaution with the return values since
> it would easily get overflowed.
>
> I scanned the entire kernel tree for all the existing callers
> and found that most of callers may get overflowed in two ways:
> either "+ 1" or passing it to ALIGN() that does "+ mask".
>
> According to kernel defines:
> #define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
> #define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
>
> We can simplify the logic here:
> ALIGN(boundary + 1, 1 << shift) >> shift
> = ALIGN_MASK(b + 1, (1 << s) - 1) >> s
> = {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
> = [b + 1 + (1 << s) - 1] >> s
> = [b + (1 << s)] >> s
> = (b >> s) + 1
>
> So this series of patches fix the potential overflow with this
> overflow-free shortcut.
Hi Nicolin,
haven't seen any other feedback from other maintainers,
so I guess you will resend this?
On first glance it seems to make sense.
I'm a little confused why it is only a "potential overflow"
while this part
"We are expending the default DMA segmentation boundary to its
possible maximum value (ULONG_MAX) to indicate that a device
doesn't specify a boundary limit"
sounds to me like ULONG_MAX is actually used, does that
mean there are currently no devices which do not specify a
boundary limit?
>
> As I don't think that I have these platforms, marking RFT.
>
> Thanks
> Nic
>
> Nicolin Chen (7):
> powerpc/iommu: Avoid overflow at boundary_size
> alpha: Avoid overflow at boundary_size
> ia64/sba_iommu: Avoid overflow at boundary_size
> s390/pci_dma: Avoid overflow at boundary_size
> sparc: Avoid overflow at boundary_size
> x86/amd_gart: Avoid overflow at boundary_size
> parisc: Avoid overflow at boundary_size
>
> arch/alpha/kernel/pci_iommu.c | 10 ++++------
> arch/ia64/hp/common/sba_iommu.c | 4 ++--
> arch/powerpc/kernel/iommu.c | 11 +++++------
> arch/s390/pci/pci_dma.c | 4 ++--
> arch/sparc/kernel/iommu-common.c | 9 +++------
> arch/sparc/kernel/iommu.c | 4 ++--
> arch/sparc/kernel/pci_sun4v.c | 4 ++--
> arch/x86/kernel/amd_gart_64.c | 4 ++--
> drivers/parisc/ccio-dma.c | 4 ++--
> drivers/parisc/sba_iommu.c | 4 ++--
> 10 files changed, 26 insertions(+), 32 deletions(-)
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [RFT][PATCH 0/7] Avoid overflow at boundary_size
2020-08-25 10:16 ` Niklas Schnelle
@ 2020-08-25 23:19 ` Nicolin Chen
0 siblings, 0 replies; 3+ messages in thread
From: Nicolin Chen @ 2020-08-25 23:19 UTC (permalink / raw)
To: Niklas Schnelle
Cc: mpe, benh, paulus, rth, ink, mattst88, tony.luck, fenghua.yu,
gerald.schaefer, hca, gor, borntraeger, davem, tglx, mingo, bp,
x86, hpa, James.Bottomley, deller, sfr, hch, linuxppc-dev,
linux-kernel, linux-alpha, linux-ia64, linux-s390, sparclinux,
linux-parisc
Hi Niklas,
On Tue, Aug 25, 2020 at 12:16:27PM +0200, Niklas Schnelle wrote:
> On 8/21/20 1:19 AM, Nicolin Chen wrote:
> > We are expending the default DMA segmentation boundary to its
> > possible maximum value (ULONG_MAX) to indicate that a device
> > doesn't specify a boundary limit. So all dma_get_seg_boundary
> > callers should take a precaution with the return values since
> > it would easily get overflowed.
> >
> > I scanned the entire kernel tree for all the existing callers
> > and found that most of callers may get overflowed in two ways:
> > either "+ 1" or passing it to ALIGN() that does "+ mask".
> >
> > According to kernel defines:
> > #define ALIGN_MASK(x, mask) (((x) + (mask)) & ~(mask))
> > #define ALIGN(x, a) ALIGN_MASK(x, (typeof(x))(a) - 1)
> >
> > We can simplify the logic here:
> > ALIGN(boundary + 1, 1 << shift) >> shift
> > = ALIGN_MASK(b + 1, (1 << s) - 1) >> s
> > = {[b + 1 + (1 << s) - 1] & ~[(1 << s) - 1]} >> s
> > = [b + 1 + (1 << s) - 1] >> s
> > = [b + (1 << s)] >> s
> > = (b >> s) + 1
> >
> > So this series of patches fix the potential overflow with this
> > overflow-free shortcut.
> haven't seen any other feedback from other maintainers,
I am wondering this too...whether I sent correctly or not.
> so I guess you will resend this?
Do I need to? Though I won't mind doing so if it's necessary..
> On first glance it seems to make sense.
> I'm a little confused why it is only a "potential overflow"
> while this part
>
> "We are expending the default DMA segmentation boundary to its
> possible maximum value (ULONG_MAX) to indicate that a device
> doesn't specify a boundary limit"
>
> sounds to me like ULONG_MAX is actually used, does that
> mean there are currently no devices which do not specify a
> boundary limit?
Sorry for the confusion. We actually applied ULONG_MAX change
last week but reverted it right after, due to a bug report at
one of these "potential" overflows. So at this moment the top
of the tree doesn't set default boundary to ULONG_MAX yet.
Thanks
Nic
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-08-25 23:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-20 23:19 [RFT][PATCH 0/7] Avoid overflow at boundary_size Nicolin Chen
2020-08-25 10:16 ` Niklas Schnelle
2020-08-25 23:19 ` Nicolin Chen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).