linux-kernel - Re: [PATCH V2 2/2] rpmsg: add syslog redirection driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 15 Feb 2019 17:06:20 +0200
From:   Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To:     xiang xiao <xiaoxiang781216@...il.com>
Cc:     Randy Dunlap <rdunlap@...radead.org>,
        Greg KH <gregkh@...uxfoundation.org>,
        alexander.shishkin@...ux.intel.com,
        Ohad Ben Cohen <ohad@...ery.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        wendy.liang@...inx.com, Arnaud POULIQUEN <arnaud.pouliquen@...com>,
        Kumar Gala <kumar.gala@...aro.org>,
        linux-remoteproc@...r.kernel.org, linux-kernel@...r.kernel.org,
        Guiding Li <liguiding@...econe.net>
Subject: Re: [PATCH V2 2/2] rpmsg: add syslog redirection driver

On Fri, Feb 15, 2019 at 12:31:17AM +0800, xiang xiao wrote:
> On Thu, Feb 14, 2019 at 9:09 PM Andy Shevchenko
> <andriy.shevchenko@...ux.intel.com> wrote:
> > On Thu, Feb 14, 2019 at 02:02:38PM +0800, Xiang Xiao wrote:

> > > This driver allows the remote processor to redirect the output of
> > > syslog or printf into the kernel log, which is very useful to see
> > > what happen in the remote side.
> >
> > > +struct rpmsg_syslog_header {
> > > +     u32                             command;
> > > +     s32                             result;
> > > +} __packed;
> >
> > Isn't packed already?
> >
> 
> But, I want to make it more explicitly and prepare for struct expansion later.

How? Why it's not in this patch / patch series?

> > > +     /* output the message before '\n' to the kernel log */
> > > +     nl = memrchr(msg->data, '\n', msg->count);
> >
> > Hmm... To me it sounds somehow fragile.
> >
> > If your text contains binary data, how can you guarantee that it would be not
> > in the middle of two \n:s?
> 
> This driver is just for log/printf redirection, so we could safely
> assume the data is pure text.

Then I don't see a point to use memrchr() at all here.

Use strchr or strrchr().

> > OTOH, if it text data, why do you need to take all strings at once?
> 
> Remote side may decide to buffer more log to reduce the IPC number
> since IPC is a time consuming operation.

So, you always can do something like

p = msg->data;
while (...strsep(..., "\n")) {
	pr_info("%s\n", token);
	...
}

> 
> >  It might be worse from performance prospective (if you know how and when printk() supplies buffer to the console).
> 
> Yes, it's very slow if the log send to serial console. But in
> production environment, printk normally just save in ram and viewed by
> dmesg which is very fast.

You may not do such assumptions. For someone it would be RAM, for some
customers it might be a slow channel.


> > > +             strncpy(priv->buf + priv->next, msg->data + printed, copied);
> >
> > Hmm... shouldn't be memcpy()?
> 
> I use memcpy initially, but found that the unaligned exception happen randomly.
> To avoid the cache issue, the IPC memory normally map as device memory, but
> ARM just allow the alignment access to this type of memory.

So, than it's an architecture level issue. With strncpy() here you will get a
pretty rightful GCC warning.

> > > +     /* flush the buffered log if need */
> > > +     if (priv->next)
> > > +             pr_info("%.*s\n", priv->next, priv->buf);
> > > +     kfree(priv->buf);
> >
> > I don't see how it's serialized. Does rpmsg core take care of this?
> 
> Yes, the callback come from a dedicated work thread.

Please, add a comment explaining that.

-- 
With Best Regards,
Andy Shevchenko