[PATCH] virtio-console: avoid DMA from vmalloc area

[PATCH] virtio-console: avoid DMA from vmalloc area

[Xianting Tian]

This patch is to optimize the commit c4baad5029:
	virtio-console: avoid DMA from stack

Commit c4baad5029(virtio-console: avoid DMA from stack) directly uses
kmemdup() to alloc new buffer from kmalloc area and do memcpy no matter
the buf is in kmalloc area or not.

This patch is to optimize the commit c4baad5029, use is_vmalloc_addr()
to judge the buf to avoid unnecessary memory alloc and copy.

Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
 drivers/char/virtio_console.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 7eaf303a7..106247903 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1119,6 +1119,7 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	struct scatterlist sg[1];
 	void *data;
 	int ret;
+	bool vmalloc_addr = false;
 
 	if (unlikely(early_put_chars))
 		return early_put_chars(vtermno, buf, count);
@@ -1127,13 +1128,18 @@ static int put_chars(u32 vtermno, const char *buf, int count)
 	if (!port)
 		return -EPIPE;
 
-	data = kmemdup(buf, count, GFP_ATOMIC);
-	if (!data)
-		return -ENOMEM;
+	if (is_vmalloc_addr(buf)) {
+		data = kmemdup(buf, count, GFP_ATOMIC);
+		if (!data)
+			return -ENOMEM;
+		vmalloc_addr = true;
+	} else
+		data = (void *)buf;
 
 	sg_init_one(sg, data, count);
 	ret = __send_to_port(port, sg, 1, count, data, false);
-	kfree(data);
+	if (vmalloc_addr)
+		kfree(data);
 	return ret;
 }
 
-- 
2.17.1

Re: [PATCH] virtio-console: avoid DMA from vmalloc area

[Arnd Bergmann]

On Tue, Jul 27, 2021 at 3:13 PM Xianting Tian
<xianting.tian@linux.alibaba.com> wrote:
> @@ -1127,13 +1128,18 @@ static int put_chars(u32 vtermno, const char *buf, int count)
>         if (!port)
>                 return -EPIPE;
>
> -       data = kmemdup(buf, count, GFP_ATOMIC);
> -       if (!data)
> -               return -ENOMEM;
> +       if (is_vmalloc_addr(buf)) {
> +               data = kmemdup(buf, count, GFP_ATOMIC);

What about buffers in .data? If those are in a loadable module, I guess you have
the same problem as with vmalloc() and vmap().

is_vmalloc_or_module_addr() would take care of both, not sure if there are
other examples that don't work. In theory it could be ioremap(), kmap_atomic()
or fixmap as well, but those seem less likely to matter here.

        Arnd

Re: [PATCH] virtio-console: avoid DMA from vmalloc area

[Xianting Tian]

Arnd, thanks for your quick reply,

As we know put_chars() of virtio-console is registered to hvc framework.
I go throughed the code, actually there are totally three places that 
put_chars() is called in hvc driver,  but only 1 has issue which is 
fixed by commit c4baad5029.
So I think the scenario that the buf is from "ioremap(), kmap_atomic() , 
fixmap, loadable module" doesn't exist for virtio-console.
If there is something wrong about above description, please correct me, 
thanks.

Three places that put_chars() is called in hvc driver:
1， it is on stack buf,  it is not ok for dma
     hvc_console_print():
         char c[N_OUTBUF] __ALIGNED__;
         cons_ops[index]->put_chars(vtermnos[index], c, i);

2， just one byte, no issue for dma
     static void hvc_poll_put_char(struct tty_driver *driver, int line, 
char ch)
     {
         struct tty_struct *tty = driver->ttys[0];
         struct hvc_struct *hp = tty->driver_data;
         int n;

         do {
             n = hp->ops->put_chars(hp->vtermno, &ch, 1);
         } while (n <= 0);
     }
3,  hp->outbuf is allocated in hvc_alloc() via kzalloc(), no issue for dma
     static int hvc_push(struct hvc_struct *hp)
     {
         int n;

         n = hp->ops->put_chars(hp->vtermno, hp->outbuf, hp->n_outbuf);
         …
     }

在 2021/7/27 下午9:18, Arnd Bergmann 写道:
> On Tue, Jul 27, 2021 at 3:13 PM Xianting Tian
> <xianting.tian@linux.alibaba.com> wrote:
>> @@ -1127,13 +1128,18 @@ static int put_chars(u32 vtermno, const char *buf, int count)
>>          if (!port)
>>                  return -EPIPE;
>>
>> -       data = kmemdup(buf, count, GFP_ATOMIC);
>> -       if (!data)
>> -               return -ENOMEM;
>> +       if (is_vmalloc_addr(buf)) {
>> +               data = kmemdup(buf, count, GFP_ATOMIC);
> What about buffers in .data? If those are in a loadable module, I guess you have
> the same problem as with vmalloc() and vmap().
>
> is_vmalloc_or_module_addr() would take care of both, not sure if there are
> other examples that don't work. In theory it could be ioremap(), kmap_atomic()
> or fixmap as well, but those seem less likely to matter here.
>
>          Arnd

Re: [PATCH] virtio-console: avoid DMA from vmalloc area

[Arnd Bergmann]

On Wed, Jul 28, 2021 at 4:59 AM Xianting Tian
<xianting.tian@linux.alibaba.com> wrote:
>
> Arnd, thanks for your quick reply,
>
> As we know put_chars() of virtio-console is registered to hvc framework.
> I go throughed the code, actually there are totally three places that
> put_chars() is called in hvc driver,  but only 1 has issue which is
> fixed by commit c4baad5029.

Ah, good. Knowing what the callers are definitely helps. ;-)

> So I think the scenario that the buf is from "ioremap(), kmap_atomic() ,
> fixmap, loadable module" doesn't exist for virtio-console.
> If there is something wrong about above description, please correct me,
> thanks.

The description is good then.

> Three places that put_chars() is called in hvc driver:
> 1， it is on stack buf,  it is not ok for dma
>      hvc_console_print():
>          char c[N_OUTBUF] __ALIGNED__;
>          cons_ops[index]->put_chars(vtermnos[index], c, i);
>
> 2， just one byte, no issue for dma
>      static void hvc_poll_put_char(struct tty_driver *driver, int line,
> char ch)
>      {
>          struct tty_struct *tty = driver->ttys[0];
>          struct hvc_struct *hp = tty->driver_data;
>          int n;
>
>          do {
>              n = hp->ops->put_chars(hp->vtermno, &ch, 1);
>          } while (n <= 0);
>      }

This is actually the same as the first, taking the address of a
function argument forces it onto the stack.

> 3,  hp->outbuf is allocated in hvc_alloc() via kzalloc(), no issue for dma
>      static int hvc_push(struct hvc_struct *hp)
>      {
>          int n;
>
>          n = hp->ops->put_chars(hp->vtermno, hp->outbuf, hp->n_outbuf);
>          …
>      }

ok.

I have a new question then: are there any other hvc backends that do
DMA, or is the virtio-console driver the only one? If there are any others,
I think this should better be fixed in the hvc framework, by changing it
to never pass stack data into the put_chars() function in the first place.

It may be possible to just use the 'hp->n_outbuf' buffer in all three cases.

       Arnd

Re: [PATCH] virtio-console: avoid DMA from vmalloc area

[Xianting Tian]

在 2021/7/28 下午3:25, Arnd Bergmann 写道:
> On Wed, Jul 28, 2021 at 4:59 AM Xianting Tian
> <xianting.tian@linux.alibaba.com> wrote:
>> Arnd, thanks for your quick reply,
>>
>> As we know put_chars() of virtio-console is registered to hvc framework.
>> I go throughed the code, actually there are totally three places that
>> put_chars() is called in hvc driver,  but only 1 has issue which is
>> fixed by commit c4baad5029.
> Ah, good. Knowing what the callers are definitely helps. ;-)
>
>> So I think the scenario that the buf is from "ioremap(), kmap_atomic() ,
>> fixmap, loadable module" doesn't exist for virtio-console.
>> If there is something wrong about above description, please correct me,
>> thanks.
> The description is good then.
>
>> Three places that put_chars() is called in hvc driver:
>> 1， it is on stack buf,  it is not ok for dma
>>       hvc_console_print():
>>           char c[N_OUTBUF] __ALIGNED__;
>>           cons_ops[index]->put_chars(vtermnos[index], c, i);
>>
>> 2， just one byte, no issue for dma
>>       static void hvc_poll_put_char(struct tty_driver *driver, int line,
>> char ch)
>>       {
>>           struct tty_struct *tty = driver->ttys[0];
>>           struct hvc_struct *hp = tty->driver_data;
>>           int n;
>>
>>           do {
>>               n = hp->ops->put_chars(hp->vtermno, &ch, 1);
>>           } while (n <= 0);
>>       }
> This is actually the same as the first, taking the address of a
> function argument forces it onto the stack.
>
>> 3,  hp->outbuf is allocated in hvc_alloc() via kzalloc(), no issue for dma
>>       static int hvc_push(struct hvc_struct *hp)
>>       {
>>           int n;
>>
>>           n = hp->ops->put_chars(hp->vtermno, hp->outbuf, hp->n_outbuf);
>>           …
>>       }
> ok.
>
> I have a new question then: are there any other hvc backends that do
> DMA, or is the virtio-console driver the only one? If there are any others,
> I think this should better be fixed in the hvc framework, by changing it
> to never pass stack data into the put_chars() function in the first place.
>
> It may be possible to just use the 'hp->n_outbuf' buffer in all three cases.

thanks,

I checked several hvc backends, like drivers/tty/hvc/hvc_riscv_sbi.c, 
drivers/tty/hvc/hvc_iucv.c, drivers/tty/hvc/hvc_rtas.c, they don't use dma.

I not finished all hvc backends check yet. But I think even if all hvc 
backends don't use dma currently, it is still possible that the hvc 
backend using dma will be added in the furture.

So I agree with you it should better be fixed in the hvc framework, 
solve the issue in the first place.

Looking forward to your reply.

>
>         Arnd

Re: [PATCH] virtio-console: avoid DMA from vmalloc area

[Arnd Bergmann]

On Wed, Jul 28, 2021 at 10:28 AM Xianting Tian
<xianting.tian@linux.alibaba.com> wrote:
> 在 2021/7/28 下午3:25, Arnd Bergmann 写道:
>
> I checked several hvc backends, like drivers/tty/hvc/hvc_riscv_sbi.c,
> drivers/tty/hvc/hvc_iucv.c, drivers/tty/hvc/hvc_rtas.c, they don't use dma.
>
> I not finished all hvc backends check yet. But I think even if all hvc
> backends don't use dma currently, it is still possible that the hvc
> backend using dma will be added in the furture.
>
> So I agree with you it should better be fixed in the hvc framework,
> solve the issue in the first place.

Ok, sounds good to me, no need to check more backends then.
I see the hvc-console driver is listed as 'Odd Fixes' in the maintainer
list, with nobody assigned other than the ppc kernel list (added to Cc).

Once you come up with a fix in hvc_console.c, please send that to the
tty maintainers, the ppc list and me, and I'll review it.

        Arnd

Re: [PATCH] virtio-console: avoid DMA from vmalloc area

[Xianting Tian]

在 2021/7/28 下午5:01, Arnd Bergmann 写道:
> On Wed, Jul 28, 2021 at 10:28 AM Xianting Tian
> <xianting.tian@linux.alibaba.com> wrote:
>> 在 2021/7/28 下午3:25, Arnd Bergmann 写道:
>>
>> I checked several hvc backends, like drivers/tty/hvc/hvc_riscv_sbi.c,
>> drivers/tty/hvc/hvc_iucv.c, drivers/tty/hvc/hvc_rtas.c, they don't use dma.
>>
>> I not finished all hvc backends check yet. But I think even if all hvc
>> backends don't use dma currently, it is still possible that the hvc
>> backend using dma will be added in the furture.
>>
>> So I agree with you it should better be fixed in the hvc framework,
>> solve the issue in the first place.
> Ok, sounds good to me, no need to check more backends then.
> I see the hvc-console driver is listed as 'Odd Fixes' in the maintainer
> list, with nobody assigned other than the ppc kernel list (added to Cc).
>
> Once you come up with a fix in hvc_console.c, please send that to the
> tty maintainers, the ppc list and me, and I'll review it.
OK, thanks, I will submit the patch ASAP :)
>
>          Arnd