LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
@ 2021-08-04  2:54 Xianting Tian
  2021-08-05  7:58 ` Jiri Slaby
  0 siblings, 1 reply; 6+ messages in thread
From: Xianting Tian @ 2021-08-04  2:54 UTC (permalink / raw)
  To: gregkh, jirislaby, amit, arnd, osandov
  Cc: linuxppc-dev, virtualization, linux-kernel, Xianting Tian

As well known, hvc backend can register its opertions to hvc backend.
the opertions contain put_chars(), get_chars() and so on.

Some hvc backend may do dma in its opertions. eg, put_chars() of
virtio-console. But in the code of hvc framework, it may pass DMA
incapable memory to put_chars() under a specific configuration, which
is explained in commit c4baad5029(virtio-console: avoid DMA from stack):
1, c[] is on stack,
   hvc_console_print():
	char c[N_OUTBUF] __ALIGNED__;
	cons_ops[index]->put_chars(vtermnos[index], c, i);
2, ch is on stack,
   static void hvc_poll_put_char(,,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);
   }

Commit c4baad5029 is just the fix to avoid DMA from stack memory, which
is passed to virtio-console by hvc framework in above code. But I think
the fix is aggressive, it directly uses kmemdup() to alloc new buffer
from kmalloc area and do memcpy no matter the memory is in kmalloc area
or not. But most importantly, it should better be fixed in the hvc
framework, by changing it to never pass stack memory to the put_chars()
function in the first place. Otherwise, we still face the same issue if
a new hvc backend using dma added in the furture.

Considering lock competition of hp->outbuf, we created a new buffer
hp->hvc_con_outbuf, which is aligned at least to N_OUTBUF, and use it
in above two cases.

With the patch, we can remove the fix c4baad5029.

Signed-off-by: Xianting Tian <xianting.tian@linux.alibaba.com>
Tested-by: Xianting Tian <xianting.tian@linux.alibaba.com>
---
 drivers/tty/hvc/hvc_console.c | 30 ++++++++++++++++++++++++++++--
 drivers/tty/hvc/hvc_console.h |  2 ++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/hvc/hvc_console.c b/drivers/tty/hvc/hvc_console.c
index 5bb8c4e44..e5862989c 100644
--- a/drivers/tty/hvc/hvc_console.c
+++ b/drivers/tty/hvc/hvc_console.c
@@ -151,9 +151,11 @@ static uint32_t vtermnos[MAX_NR_HVC_CONSOLES] =
 static void hvc_console_print(struct console *co, const char *b,
 			      unsigned count)
 {
-	char c[N_OUTBUF] __ALIGNED__;
+	char *c;
 	unsigned i = 0, n = 0;
 	int r, donecr = 0, index = co->index;
+	unsigned long flags;
+	struct hvc_struct *hp;
 
 	/* Console access attempt outside of acceptable console range. */
 	if (index >= MAX_NR_HVC_CONSOLES)
@@ -163,6 +165,13 @@ static void hvc_console_print(struct console *co, const char *b,
 	if (vtermnos[index] == -1)
 		return;
 
+	list_for_each_entry(hp, &hvc_structs, next)
+		if (hp->vtermno == vtermnos[index])
+			break;
+
+	c = hp->hvc_con_outbuf;
+
+	spin_lock_irqsave(&hp->hvc_con_lock, flags);
 	while (count > 0 || i > 0) {
 		if (count > 0 && i < sizeof(c)) {
 			if (b[n] == '\n' && !donecr) {
@@ -191,6 +200,7 @@ static void hvc_console_print(struct console *co, const char *b,
 			}
 		}
 	}
+	spin_unlock_irqrestore(&hp->hvc_con_lock, flags);
 	hvc_console_flush(cons_ops[index], vtermnos[index]);
 }
 
@@ -878,9 +888,15 @@ 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;
+	unsigned long flags;
+	char *c;
 
+	c = hp->hvc_con_outbuf;
 	do {
-		n = hp->ops->put_chars(hp->vtermno, &ch, 1);
+		spin_lock_irqsave(&hp->hvc_con_lock, flags);
+		c[0] = ch;
+		n = hp->ops->put_chars(hp->vtermno, c, 1);
+		spin_unlock_irqrestore(&hp->hvc_con_lock, flags);
 	} while (n <= 0);
 }
 #endif
@@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
 	hp->outbuf_size = outbuf_size;
 	hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
 
+	/*
+	 * hvc_con_outbuf is guaranteed to be aligned at least to the
+	 * size(N_OUTBUF) by kmalloc().
+	 */
+	hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL);
+	if (!hp->hvc_con_outbuf)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock_init(&hp->hvc_con_lock);
+
 	tty_port_init(&hp->port);
 	hp->port.ops = &hvc_port_ops;
 
diff --git a/drivers/tty/hvc/hvc_console.h b/drivers/tty/hvc/hvc_console.h
index 18d005814..8972c52de 100644
--- a/drivers/tty/hvc/hvc_console.h
+++ b/drivers/tty/hvc/hvc_console.h
@@ -48,6 +48,8 @@ struct hvc_struct {
 	struct work_struct tty_resize;
 	struct list_head next;
 	unsigned long flags;
+	char *hvc_con_outbuf;
+	spinlock_t hvc_con_lock;
 };
 
 /* implemented by a low level driver */
-- 
2.17.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
  2021-08-04  2:54 [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian
@ 2021-08-05  7:58 ` Jiri Slaby
  2021-08-05  8:09   ` Jiri Slaby
       [not found]   ` <40f78d10-0a57-4620-e7e2-f806bd61abca@linux.alibaba.com>
  0 siblings, 2 replies; 6+ messages in thread
From: Jiri Slaby @ 2021-08-05  7:58 UTC (permalink / raw)
  To: Xianting Tian, gregkh, amit, arnd, osandov
  Cc: linuxppc-dev, virtualization, linux-kernel

Hi,

On 04. 08. 21, 4:54, Xianting Tian wrote:
> @@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, int data,
>   	hp->outbuf_size = outbuf_size;
>   	hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
>   
> +	/*
> +	 * hvc_con_outbuf is guaranteed to be aligned at least to the
> +	 * size(N_OUTBUF) by kmalloc().
> +	 */
> +	hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL);
> +	if (!hp->hvc_con_outbuf)
> +		return ERR_PTR(-ENOMEM);

This leaks hp, right?

BTW your 2 patches are still not threaded, that is hard to follow.

> +
> +	spin_lock_init(&hp->hvc_con_lock);
> +
>   	tty_port_init(&hp->port);
>   	hp->port.ops = &hvc_port_ops;
>   

thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
  2021-08-05  7:58 ` Jiri Slaby
@ 2021-08-05  8:09   ` Jiri Slaby
  2021-08-05 11:14     ` Xianting Tian
       [not found]   ` <40f78d10-0a57-4620-e7e2-f806bd61abca@linux.alibaba.com>
  1 sibling, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2021-08-05  8:09 UTC (permalink / raw)
  To: Xianting Tian, gregkh, amit, arnd, osandov
  Cc: linuxppc-dev, virtualization, linux-kernel

On 05. 08. 21, 9:58, Jiri Slaby wrote:
> Hi,
> 
> On 04. 08. 21, 4:54, Xianting Tian wrote:
>> @@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, 
>> int data,
>>       hp->outbuf_size = outbuf_size;
>>       hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];

This deserves cleanup too. Why is "outbuf" not "char outbuf[0] 
__ALIGNED__" at the end of the structure? The allocation would be easier 
(using struct_size()) and this line would be gone completely.

>> +    /*
>> +     * hvc_con_outbuf is guaranteed to be aligned at least to the
>> +     * size(N_OUTBUF) by kmalloc().
>> +     */
>> +    hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL);
>> +    if (!hp->hvc_con_outbuf)
>> +        return ERR_PTR(-ENOMEM);
> 
> This leaks hp, right?

Actually, why don't you make
char c[N_OUTBUF] __ALIGNED__;

part of struct hvc_struct directly?

> BTW your 2 patches are still not threaded, that is hard to follow.
> 
>> +
>> +    spin_lock_init(&hp->hvc_con_lock);
>> +
>>       tty_port_init(&hp->port);
>>       hp->port.ops = &hvc_port_ops;
> 
> thanks,
-- 
js
suse labs

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
       [not found]   ` <40f78d10-0a57-4620-e7e2-f806bd61abca@linux.alibaba.com>
@ 2021-08-05  8:18     ` Greg KH
  2021-08-05  8:23       ` Xianting Tian
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2021-08-05  8:18 UTC (permalink / raw)
  To: Xianting Tian
  Cc: Jiri Slaby, amit, arnd, osandov, linuxppc-dev, virtualization,
	linux-kernel

On Thu, Aug 05, 2021 at 04:08:46PM +0800, Xianting Tian wrote:
> 
> 在 2021/8/5 下午3:58, Jiri Slaby 写道:
> > Hi,
> > 
> > On 04. 08. 21, 4:54, Xianting Tian wrote:
> > > @@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno,
> > > int data,
> > >       hp->outbuf_size = outbuf_size;
> > >       hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
> > >   +    /*
> > > +     * hvc_con_outbuf is guaranteed to be aligned at least to the
> > > +     * size(N_OUTBUF) by kmalloc().
> > > +     */
> > > +    hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL);
> > > +    if (!hp->hvc_con_outbuf)
> > > +        return ERR_PTR(-ENOMEM);
> > 
> > This leaks hp, right?
> > 
> > BTW your 2 patches are still not threaded, that is hard to follow.
> 
> yes, thanks, I found the bug, I am preparing to do this in v4.
> 
> It is the first time I send series patches(number >1), I checked the method
> for sending series patch on LKML.org, I should send '0/2' which is the
> history info for series patches.

Please use 'git send-email' to send the full series all at once,
otherwise it is hard to make the emails threaded "by hand" if you do not
do so.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
  2021-08-05  8:18     ` Greg KH
@ 2021-08-05  8:23       ` Xianting Tian
  0 siblings, 0 replies; 6+ messages in thread
From: Xianting Tian @ 2021-08-05  8:23 UTC (permalink / raw)
  To: Greg KH
  Cc: Jiri Slaby, amit, arnd, osandov, linuxppc-dev, virtualization,
	linux-kernel


在 2021/8/5 下午4:18, Greg KH 写道:
> On Thu, Aug 05, 2021 at 04:08:46PM +0800, Xianting Tian wrote:
>> 在 2021/8/5 下午3:58, Jiri Slaby 写道:
>>> Hi,
>>>
>>> On 04. 08. 21, 4:54, Xianting Tian wrote:
>>>> @@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno,
>>>> int data,
>>>>        hp->outbuf_size = outbuf_size;
>>>>        hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
>>>>    +    /*
>>>> +     * hvc_con_outbuf is guaranteed to be aligned at least to the
>>>> +     * size(N_OUTBUF) by kmalloc().
>>>> +     */
>>>> +    hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL);
>>>> +    if (!hp->hvc_con_outbuf)
>>>> +        return ERR_PTR(-ENOMEM);
>>> This leaks hp, right?
>>>
>>> BTW your 2 patches are still not threaded, that is hard to follow.
>> yes, thanks, I found the bug, I am preparing to do this in v4.
>>
>> It is the first time I send series patches(number >1), I checked the method
>> for sending series patch on LKML.org, I should send '0/2' which is the
>> history info for series patches.
> Please use 'git send-email' to send the full series all at once,
> otherwise it is hard to make the emails threaded "by hand" if you do not
> do so.
I got it, thanks for your guide:)
>
> thanks,
>
> greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars()
  2021-08-05  8:09   ` Jiri Slaby
@ 2021-08-05 11:14     ` Xianting Tian
  0 siblings, 0 replies; 6+ messages in thread
From: Xianting Tian @ 2021-08-05 11:14 UTC (permalink / raw)
  To: Jiri Slaby, gregkh, amit, arnd, osandov
  Cc: linuxppc-dev, virtualization, linux-kernel


在 2021/8/5 下午4:09, Jiri Slaby 写道:
> On 05. 08. 21, 9:58, Jiri Slaby wrote:
>> Hi,
>>
>> On 04. 08. 21, 4:54, Xianting Tian wrote:
>>> @@ -933,6 +949,16 @@ struct hvc_struct *hvc_alloc(uint32_t vtermno, 
>>> int data,
>>>       hp->outbuf_size = outbuf_size;
>>>       hp->outbuf = &((char *)hp)[ALIGN(sizeof(*hp), sizeof(long))];
>
> This deserves cleanup too. Why is "outbuf" not "char outbuf[0] 
> __ALIGNED__" at the end of the structure? The allocation would be 
> easier (using struct_size()) and this line would be gone completely.
I will make the cleanup in v4.
>
>>> +    /*
>>> +     * hvc_con_outbuf is guaranteed to be aligned at least to the
>>> +     * size(N_OUTBUF) by kmalloc().
>>> +     */
>>> +    hp->hvc_con_outbuf = kzalloc(N_OUTBUF, GFP_KERNEL);
>>> +    if (!hp->hvc_con_outbuf)
>>> +        return ERR_PTR(-ENOMEM);
>>
>> This leaks hp, right?
>
> Actually, why don't you make
> char c[N_OUTBUF] __ALIGNED__;
>
> part of struct hvc_struct directly?
thanks, it a good idea, I will change it in v4.
>
>> BTW your 2 patches are still not threaded, that is hard to follow.
>>
>>> +
>>> +    spin_lock_init(&hp->hvc_con_lock);
>>> +
>>>       tty_port_init(&hp->port);
>>>       hp->port.ops = &hvc_port_ops;
>>
>> thanks,

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-08-05 11:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-04  2:54 [PATCH v3 1/2] tty: hvc: pass DMA capable memory to put_chars() Xianting Tian
2021-08-05  7:58 ` Jiri Slaby
2021-08-05  8:09   ` Jiri Slaby
2021-08-05 11:14     ` Xianting Tian
     [not found]   ` <40f78d10-0a57-4620-e7e2-f806bd61abca@linux.alibaba.com>
2021-08-05  8:18     ` Greg KH
2021-08-05  8:23       ` Xianting Tian

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).