KERN_<LEVEL> currently takes up 3 bytes. Shrink the kernel size by using an ASCII SOH and then the level byte. Remove the need for KERN_CONT. Convert directly embedded uses of <.> to KERN_<LEVEL> Joe Perches (8): printk: Add generic functions to find KERN_<LEVEL> headers printk: Add kern_levels.h to make KERN_<LEVEL> available for asm use arch: Remove direct definitions of KERN_<LEVEL> uses btrfs: Use printk_get_level and printk_skip_level, add __printf, fix fallout sound: Use printk_get_level and printk_skip_level staging: wlags49_h2: Remove direct declarations of KERN_<LEVEL> prefixes printk: Convert the format for KERN_<LEVEL> to a 2 byte pattern printk: Remove the now unnecessary "C" annotation for KERN_CONT arch/arm/lib/io-acorn.S | 3 +- arch/arm/vfp/vfphw.S | 7 +++-- arch/frv/kernel/kernel_thread.S | 2 +- drivers/staging/wlags49_h2/hcf.c | 8 +++--- drivers/staging/wlags49_h2/wl_main.c | 4 +- fs/btrfs/ctree.h | 13 ++++++++++ fs/btrfs/disk-io.c | 2 +- fs/btrfs/relocation.c | 2 +- fs/btrfs/super.c | 41 +++++++++++++++++++++++++++++----- include/linux/kern_levels.h | 25 ++++++++++++++++++++ include/linux/printk.h | 41 +++++++++++++++++++-------------- kernel/printk.c | 14 +++++++---- sound/core/misc.c | 13 +++++++--- 13 files changed, 130 insertions(+), 45 deletions(-) create mode 100644 include/linux/kern_levels.h -- 1.7.8.111.gad25c.dirty
Joe Perches
2012-Jun-05 09:46 UTC
[PATCH 4/8] btrfs: Use printk_get_level and printk_skip_level, add __printf, fix fallout
Use the generic printk_get_level function to search a message
for a kern_level. Add __printf to verify format and arguments.
Fix a few messages that had mismatches in format and arguments.
Add #ifdef CONFIG_PRINTK blocks to shrink the object size a bit
when not using printk.
Signed-off-by: Joe Perches <joe@perches.com>
---
fs/btrfs/ctree.h | 13 +++++++++++++
fs/btrfs/disk-io.c | 2 +-
fs/btrfs/relocation.c | 2 +-
fs/btrfs/super.c | 41 +++++++++++++++++++++++++++++++++++------
4 files changed, 50 insertions(+), 8 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 0236d03..590b519 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -3046,10 +3046,22 @@ ssize_t btrfs_listxattr(struct dentry *dentry, char
*buffer, size_t size);
/* super.c */
int btrfs_parse_options(struct btrfs_root *root, char *options);
int btrfs_sync_fs(struct super_block *sb, int wait);
+
+#ifdef CONFIG_PRINTK
+__printf(2, 3)
void btrfs_printk(struct btrfs_fs_info *fs_info, const char *fmt, ...);
+#else
+static inline __printf(2, 3)
+void btrfs_printk(struct btrfs_fs_info *fs_info, const char *fmt, ...)
+{
+}
+#endif
+
+__printf(5, 6)
void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
unsigned int line, int errno, const char *fmt, ...);
+
void __btrfs_abort_transaction(struct btrfs_trans_handle *trans,
struct btrfs_root *root, const char *function,
unsigned int line, int errno);
@@ -3073,6 +3085,7 @@ do { \
(errno), fmt, ##args); \
} while (0)
+__printf(5, 6)
void __btrfs_panic(struct btrfs_fs_info *fs_info, const char *function,
unsigned int line, int errno, const char *fmt, ...);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 7ae51de..f02bba5 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1113,7 +1113,7 @@ void clean_tree_block(struct btrfs_trans_handle *trans,
struct btrfs_root *root,
spin_unlock(&root->fs_info->delalloc_lock);
btrfs_panic(root->fs_info, -EOVERFLOW,
"Can''t clear %lu bytes from "
- " dirty_mdatadata_bytes (%lu)",
+ " dirty_mdatadata_bytes (%llu)",
buf->len,
root->fs_info->dirty_metadata_bytes);
}
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 646ee21..790f492 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1242,7 +1242,7 @@ static int __must_check __add_reloc_root(struct btrfs_root
*root)
kfree(node);
btrfs_panic(root->fs_info, -EEXIST, "Duplicate root found "
"for start=%llu while inserting into relocation "
- "tree\n");
+ "tree\n", node->bytenr);
}
list_add_tail(&root->root_list, &rc->reloc_roots);
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 96eb9fe..a84529d 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -124,6 +124,7 @@ static void btrfs_handle_error(struct btrfs_fs_info
*fs_info)
}
}
+#ifdef CONFIG_PRINTK
/*
* __btrfs_std_error decodes expected errors from the caller and
* invokes the approciate error response.
@@ -166,7 +167,7 @@ void __btrfs_std_error(struct btrfs_fs_info *fs_info, const
char *function,
va_end(args);
}
-const char *logtypes[] = {
+static const char *logtypes[] = {
"emergency",
"alert",
"critical",
@@ -184,22 +185,50 @@ void btrfs_printk(struct btrfs_fs_info *fs_info, const
char *fmt, ...)
struct va_format vaf;
va_list args;
const char *type = logtypes[4];
+ int kern_level;
va_start(args, fmt);
- if (fmt[0] == ''<'' && isdigit(fmt[1]) &&
fmt[2] == ''>'') {
- memcpy(lvl, fmt, 3);
- lvl[3] = ''\0'';
- fmt += 3;
- type = logtypes[fmt[1] - ''0''];
+ kern_level = printk_get_level(fmt);
+ if (kern_level) {
+ size_t size = printk_skip_level(fmt) - fmt;
+ memcpy(lvl, fmt, size);
+ lvl[size] = ''\0'';
+ fmt += size;
+ type = logtypes[kern_level - ''0''];
} else
*lvl = ''\0'';
vaf.fmt = fmt;
vaf.va = &args;
+
printk("%sBTRFS %s (device %s): %pV", lvl, type, sb->s_id,
&vaf);
+
+ va_end(args);
}
+#else
+
+void __btrfs_std_error(struct btrfs_fs_info *fs_info, const char *function,
+ unsigned int line, int errno, const char *fmt, ...)
+{
+ struct super_block *sb = fs_info->sb;
+
+ /*
+ * Special case: if the error is EROFS, and we''re already
+ * under MS_RDONLY, then it is safe here.
+ */
+ if (errno == -EROFS && (sb->s_flags & MS_RDONLY))
+ return;
+
+ /* Don''t go through full error handling during mount */
+ if (sb->s_flags & MS_BORN) {
+ save_error_info(fs_info);
+ btrfs_handle_error(fs_info);
+ }
+}
+#endif
+
/*
* We only mark the transaction aborted and then set the file system read-only.
* This will prevent new transactions from starting or trying to join this
--
1.7.8.111.gad25c.dirty
On Tue, 5 Jun 2012 02:46:29 -0700 Joe Perches <joe@perches.com> wrote:> KERN_<LEVEL> currently takes up 3 bytes. > Shrink the kernel size by using an ASCII SOH and then the level byte. > Remove the need for KERN_CONT. > Convert directly embedded uses of <.> to KERN_<LEVEL>What an epic patchset. I guess that saving a byte per printk does make the world a better place, and forcibly ensuring that nothing is dependent upon the internal format of the KERN_foo strings is nice. Unfortunately the <n> thing is part of the kernel ABI: echo "<4>foo" > /dev/kmsg devkmsg_writev() does weird and wonderful things with facilities/levels. That function incorrectly returns "success" when copy_from_user() faults, btw. It also babbles on about LOG_USER and LOG_KERN without ever defining these things. I guess they''re userspace-only concepts and are hardwired to 0 and 1 in the kernel. Or not. So what to do about /dev/kmsg? I''d say "nothing": we retain "<n>" as the externally-presented kernel format for a facility level, and the fact that the kernel internally uses a different encoding is hidden from userspace. And if the user does echo "\0014foo" > /dev/kmsg then I guess we should pass it straight through, retaining the \0014. But from my reading of your code, this doesn''t work - vprintk_emit() will go ahead and strip and interpret the \0014, evading the stuff which devkmsg_writev() did.
On Tue, Jun 5, 2012 at 11:28 PM, Andrew Morton <akpm@linux-foundation.org> wrote:> On Tue, 5 Jun 2012 02:46:29 -0700 > Joe Perches <joe@perches.com> wrote: > >> KERN_<LEVEL> currently takes up 3 bytes. >> Shrink the kernel size by using an ASCII SOH and then the level byte. >> Remove the need for KERN_CONT. >> Convert directly embedded uses of <.> to KERN_<LEVEL> > > What an epic patchset. I guess that saving a byte per printk does make > the world a better place, and forcibly ensuring that nothing is > dependent upon the internal format of the KERN_foo strings is nice. > > > Unfortunately the <n> thing is part of the kernel ABI: > > echo "<4>foo" > /dev/kmsg > > devkmsg_writev() does weird and wonderful things with > facilities/levels. That function incorrectly returns "success" when > copy_from_user() faults, btw. It also babbles on about LOG_USER and > LOG_KERN without ever defining these things. I guess they''re > userspace-only concepts and are hardwired to 0 and 1 in the kernel. Or > not.It''s as old as BSD, defined by syslog(3), used by glibc. The whole <%u> prefix notation and the LOG_* names come from there. The kernel is just user/facility == 0, so it never really was apparent that the whole concept has more than a log level in that number. Userspace syslog defines these pretty stupid numbers: /* facility codes */ #define LOG_KERN (0<<3) /* kernel messages */ #define LOG_USER (1<<3) /* random user-level messages */ #define LOG_MAIL (2<<3) /* mail system */ #define LOG_DAEMON (3<<3) /* system daemons */ #define LOG_AUTH (4<<3) /* security/authorization messages */ #define LOG_SYSLOG (5<<3) /* messages generated internally by syslogd */ #define LOG_LPR (6<<3) /* line printer subsystem */ #define LOG_NEWS (7<<3) /* network news subsystem */ #define LOG_UUCP (8<<3) /* UUCP subsystem */ #define LOG_CRON (9<<3) /* clock daemon */ #define LOG_AUTHPRIV (10<<3) /* security/authorization messages (private) */ #define LOG_FTP (11<<3) /* ftp daemon */ but it *can* still all be pretty useful, and people *can* get creative with facility numbers if they want to, as we have like 13 bits at the moment to use for the facility which is stored in the kernel log buffer. :) /dev/kmsg just enforces LOG_USER, if userspace tries to inject stuff with LOG_KERN, which it should not be allowed. The non-LOG_KERN number itself has not much meaning it just says: "this is not from the kernel" which is important to keep in the message. Als, dmesg(1) has a -k option, that filters out all userspace-injected stuff.> So what to do about /dev/kmsg? I''d say "nothing": we retain "<n>" as > the externally-presented kernel format for a facility level, and the > fact that the kernel internally uses a different encoding is hidden > from userspace.Yeah, I think so. Yeah, we strip the <> at printk() time, add the <> back at output time; they are not stored internally anymore, so that should not affect the current behaviour.> And if the user does > > echo "\0014foo" > /dev/kmsg > > then I guess we should pass it straight through, retaining the \0014. > But from my reading of your code, this doesn''t work - vprintk_emit() > will go ahead and strip and interpret the \0014, evading the stuff > which devkmsg_writev() did.We should make it not accept faked prefixes, yes. It should be impossible to let messages look like they originated from the kernel, just like the current code enforces a non-LOG_KERN <> prefix. Kay -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2012-06-05 at 14:28 -0700, Andrew Morton wrote:> Unfortunately the <n> thing is part of the kernel ABI: > > echo "<4>foo" > /dev/kmsgWhich works the same way it did before.
On Tue, 05 Jun 2012 15:11:43 -0700 Joe Perches <joe@perches.com> wrote:> On Tue, 2012-06-05 at 14:28 -0700, Andrew Morton wrote: > > Unfortunately the <n> thing is part of the kernel ABI: > > > > echo "<4>foo" > /dev/kmsg > > Which works the same way it did before.I didn''t say it didn''t. What I did say is that echo "\0014">/dev/kmsg will subvert the intent of the new logging code. Or might. But you just ignored all that, forcing me to repeat myself, irritatedly.
On Tue, 2012-06-05 at 15:17 -0700, Andrew Morton wrote:> On Tue, 05 Jun 2012 15:11:43 -0700 > Joe Perches <joe@perches.com> wrote: > > > On Tue, 2012-06-05 at 14:28 -0700, Andrew Morton wrote: > > > Unfortunately the <n> thing is part of the kernel ABI: > > > > > > echo "<4>foo" > /dev/kmsg > > > > Which works the same way it did before. > > I didn''t say it didn''t. > > What I did say is that echo "\0014">/dev/kmsg will subvert the intent > of the new logging code. Or might. But you just ignored all that, > forcing me to repeat myself, irritatedly.It works the same way before and after the patch. Any write to /dev/kmsg without a KERN_<LEVEL> emits at (1 << 3) + KERN_DEFAULT. Writes with <n> values >= 8 are emitted at that level.
On Tue, 2012-06-05 at 14:28 -0700, Andrew Morton wrote:> devkmsg_writev() does weird and wonderful things with > facilities/levels. That function incorrectly returns "success" when > copy_from_user() faults, btw.Oh. Better? Thanks, Kay From: Kay Sievers <kay@vrfy.org> Subject: kmsg: /dev/kmsg - properly return possible copy_from_user() failure Reported-By: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Kay Sievers <kay@vrfy.org --- printk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kernel/printk.c b/kernel/printk.c index 32462d2..6bdacab 100644 --- a/kernel/printk.c +++ b/kernel/printk.c @@ -365,8 +365,10 @@ static ssize_t devkmsg_writev(struct kiocb *iocb, const struct iovec *iv, line = buf; for (i = 0; i < count; i++) { - if (copy_from_user(line, iv[i].iov_base, iv[i].iov_len)) + if (copy_from_user(line, iv[i].iov_base, iv[i].iov_len)) { + ret = -EFAULT; goto out; + } line += iv[i].iov_len; }
On Wed, 06 Jun 2012 00:55:00 +0200 Kay Sievers <kay@vrfy.org> wrote:> On Tue, 2012-06-05 at 14:28 -0700, Andrew Morton wrote: > > > devkmsg_writev() does weird and wonderful things with > > facilities/levels. That function incorrectly returns "success" when > > copy_from_user() faults, btw. > > Oh. Better? > > Thanks, > Kay > > > From: Kay Sievers <kay@vrfy.org> > Subject: kmsg: /dev/kmsg - properly return possible copy_from_user() failure > > Reported-By: Andrew Morton <akpm@linux-foundation.org> > Signed-off-by: Kay Sievers <kay@vrfy.org > --- > printk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/kernel/printk.c b/kernel/printk.c > index 32462d2..6bdacab 100644 > --- a/kernel/printk.c > +++ b/kernel/printk.c > @@ -365,8 +365,10 @@ static ssize_t devkmsg_writev(struct kiocb *iocb, const struct iovec *iv, > > line = buf; > for (i = 0; i < count; i++) { > - if (copy_from_user(line, iv[i].iov_base, iv[i].iov_len)) > + if (copy_from_user(line, iv[i].iov_base, iv[i].iov_len)) { > + ret = -EFAULT; > goto out; > + } > line += iv[i].iov_len; > }Strictly speaking, when write() encounters an error it should return number-of-bytes-written, or an errno if it wrote zero bytes. So something like --- a/kernel/printk.c~a +++ a/kernel/printk.c @@ -355,7 +355,7 @@ static ssize_t devkmsg_writev(struct kio int level = default_message_loglevel; int facility = 1; /* LOG_USER */ size_t len = iov_length(iv, count); - ssize_t ret = len; + ssize_t ret; if (len > LOG_LINE_MAX) return -EINVAL; @@ -365,8 +365,12 @@ static ssize_t devkmsg_writev(struct kio line = buf; for (i = 0; i < count; i++) { - if (copy_from_user(line, iv[i].iov_base, iv[i].iov_len)) + if (copy_from_user(line, iv[i].iov_base, iv[i].iov_len)) { + ret = line - buf; + if (!ret) + ret = -EFAULT; goto out; + } line += iv[i].iov_len; } @@ -396,6 +400,7 @@ static ssize_t devkmsg_writev(struct kio line[len] = ''\0''; printk_emit(facility, level, NULL, 0, "%s", line); + ret = 0; out: kfree(buf); return ret; _
On Tue, 05 Jun 2012 15:49:32 -0700 Joe Perches <joe@perches.com> wrote:> On Tue, 2012-06-05 at 15:17 -0700, Andrew Morton wrote: > > On Tue, 05 Jun 2012 15:11:43 -0700 > > Joe Perches <joe@perches.com> wrote: > > > > > On Tue, 2012-06-05 at 14:28 -0700, Andrew Morton wrote: > > > > Unfortunately the <n> thing is part of the kernel ABI: > > > > > > > > echo "<4>foo" > /dev/kmsg > > > > > > Which works the same way it did before. > > > > I didn''t say it didn''t. > > > > What I did say is that echo "\0014">/dev/kmsg will subvert the intent > > of the new logging code. Or might. But you just ignored all that, > > forcing me to repeat myself, irritatedly. > > It works the same way before and after the patch. > > Any write to /dev/kmsg without a KERN_<LEVEL> > emits at (1 << 3) + KERN_DEFAULT. > > Writes with <n> values >= 8 are emitted at > that level.What about writes starting with \001n? AFACIT, that will be stripped away and the printk level will be altered. This is new behavior.
On Tue, 2012-06-05 at 16:29 -0700, Andrew Morton wrote:> What about writes starting with \001n? AFACIT, that will be stripped > away and the printk level will be altered. This is new behavior.Nope. # echo "\001Hello Andrew" > /dev/kmsg /dev/kmsg has 12,774,2462339252;\001Hello Andrew 12 is 8 + KERN_DEFAULT # echo "<1>Hello Andrew" > /dev/kmsg /dev/kmsg has: 9,775,2516023444;Hello Andrew
On Wed, Jun 6, 2012 at 1:35 AM, Joe Perches <joe@perches.com> wrote:> On Tue, 2012-06-05 at 16:29 -0700, Andrew Morton wrote: >> What about writes starting with \001n? AFACIT, that will be stripped >> away and the printk level will be altered. This is new behavior. > > Nope. > > # echo "\001Hello Andrew" > /dev/kmsg > /dev/kmsg has > 12,774,2462339252;\001Hello AndrewTry "echo -e"? The stuff is copied verbatim otherwise. Kay
On Wed, 2012-06-06 at 01:39 +0200, Kay Sievers wrote:> On Wed, Jun 6, 2012 at 1:35 AM, Joe Perches <joe@perches.com> wrote: > > On Tue, 2012-06-05 at 16:29 -0700, Andrew Morton wrote: > >> What about writes starting with \001n? AFACIT, that will be stripped > >> away and the printk level will be altered. This is new behavior. > > > > Nope. > > > > # echo "\001Hello Andrew" > /dev/kmsg > > /dev/kmsg has > > 12,774,2462339252;\001Hello Andrew > > Try "echo -e"? The stuff is copied verbatim otherwise.# echo -e "\001Hello Kay" > /dev/kmsg gives 12,776,3046752764;\x01Hello Kay -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-06-06 at 01:39 +0200, Kay Sievers wrote:> Try "echo -e"? The stuff is copied verbatim otherwise.btw: I didn''t change the devkmsg_writev function which does the decoding of any "<n>" prefix for printk_emit.
On Wed, Jun 6, 2012 at 1:43 AM, Joe Perches <joe@perches.com> wrote:> On Wed, 2012-06-06 at 01:39 +0200, Kay Sievers wrote:>> > # echo "\001Hello Andrew" > /dev/kmsg >> > /dev/kmsg has >> > 12,774,2462339252;\001Hello Andrew >> >> Try "echo -e"? The stuff is copied verbatim otherwise. > > # echo -e "\001Hello Kay" > /dev/kmsg > gives > 12,776,3046752764;\x01Hello KayDon''t you need two bytes to trigger the logic? Kay
On Wed, 2012-06-06 at 01:48 +0200, Kay Sievers wrote:> On Wed, Jun 6, 2012 at 1:43 AM, Joe Perches <joe@perches.com> wrote: > > On Wed, 2012-06-06 at 01:39 +0200, Kay Sievers wrote: > > >> > # echo "\001Hello Andrew" > /dev/kmsg > >> > /dev/kmsg has > >> > 12,774,2462339252;\001Hello Andrew > >> > >> Try "echo -e"? The stuff is copied verbatim otherwise. > > > > # echo -e "\001Hello Kay" > /dev/kmsg > > gives > > 12,776,3046752764;\x01Hello Kay > > Don''t you need two bytes to trigger the logic?Yes. Angle brackets fore and aft. If you use a "<n>" prefix for write to /dev/kmsg, then it''s supported just like before. Otherwise, it''s emitted at KERN_DEFAULT. cheers, Joe
On Tue, 05 Jun 2012 16:52:25 -0700 Joe Perches <joe@perches.com> wrote:> On Wed, 2012-06-06 at 01:48 +0200, Kay Sievers wrote: > > On Wed, Jun 6, 2012 at 1:43 AM, Joe Perches <joe@perches.com> wrote: > > > On Wed, 2012-06-06 at 01:39 +0200, Kay Sievers wrote: > > > > >> > # echo "\001Hello Andrew" > /dev/kmsg > > >> > /dev/kmsg has > > >> > 12,774,2462339252;\001Hello Andrew > > >> > > >> Try "echo -e"? The stuff is copied verbatim otherwise. > > > > > > # echo -e "\001Hello Kay" > /dev/kmsg > > > gives > > > 12,776,3046752764;\x01Hello Kay > > > > Don''t you need two bytes to trigger the logic? > > Yes. Angle brackets fore and aft.he means echo "\0014Hello Joe" > /dev/kmsg ^ It needs one of [0-7cd] to trigger the prefix handling.
On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote:> echo "\0014Hello Joe" > /dev/kmsg# echo -e "\x014Hello Me" > /dev/kmsg gives: 12,778,4057982669;Hello Me #echo -e "\x011Hello Me_2" > /dev/kmsg gives: 12,779,4140452093;Hello Me_2 I didn''t change devkmsg_writev so the original parsing style for "<.>" is unchanged. from printk.c: static ssize_t devkmsg_writev(struct kiocb *iocb, const struct iovec *iv, unsigned long count, loff_t pos) [...] int level = default_message_loglevel; [...] if (line[0] == ''<'') { char *endp = NULL; i = simple_strtoul(line+1, &endp, 10); if (endp && endp[0] == ''>'') { level = i & 7; if (i >> 3) facility = i >> 3; endp++; len -= endp - line; line = endp; } } line[len] = ''\0''; printk_emit(facility, level, NULL, 0, "%s", line); [] level is what matters. from dmesg -r <12>[ 2462.339252] \001Hello Andrew <9>[ 2516.023444] Hello Andrew <12>[ 3046.752764] \x01Hello Kay <12>[ 3940.871850] \x01Hello Kay <12>[ 4057.982669] Hello Me <12>[ 4140.452093] Hello Me_2
On Wed, Jun 6, 2012 at 2:07 AM, Joe Perches <joe@perches.com> wrote:> On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote: >> echo "\0014Hello Joe" > /dev/kmsg > > # echo -e "\x014Hello Me" > /dev/kmsg > gives: > 12,778,4057982669;Hello Me > > #echo -e "\x011Hello Me_2" > /dev/kmsg > gives: > 12,779,4140452093;Hello Me_2 > > I didn't change devkmsg_writev so the > original parsing style for "<.>" is > unchanged. > > from printk.c: > > static ssize_t devkmsg_writev(struct kiocb *iocb, const struct iovec *iv, > unsigned long count, loff_t pos) > [...] > int level = default_message_loglevel; > [...] > if (line[0] == '<') { > char *endp = NULL; > > i = simple_strtoul(line+1, &endp, 10); > if (endp && endp[0] == '>') { > level = i & 7; > if (i >> 3) > facility = i >> 3; > endp++; > len -= endp - line; > line = endp; > } > } > line[len] = '\0'; > > printk_emit(facility, level, NULL, 0, "%s", line); > [] > > level is what matters. > > from dmesg -r > > <12>[ 2462.339252] \001Hello Andrew > <9>[ 2516.023444] Hello Andrew > <12>[ 3046.752764] \x01Hello Kay > <12>[ 3940.871850] \x01Hello Kay > <12>[ 4057.982669] Hello Me > <12>[ 4140.452093] Hello Me_2The question is what happens if you inject your new binary two-byte prefix, like: echo -e "\x01\x02Hello" > /dev/kmsg And if that changes the log-level to "2" instead of the default "4"? (assuming that I read your patch right, otherwise please correct the bytes, but use the full sequence which your patch will recognize as an internal level marker; seems your examples are all not triggering that) Kay _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
On Wed, 2012-06-06 at 02:13 +0200, Kay Sievers wrote:> The question is what happens if you inject your new binary two-byte > prefix, like: > echo -e "\x01\x02Hello" > /dev/kmsgIt''s not a 2 byte binary. It''s a leading ascii SOH and a standard ascii char ''0'' ... ''7'' or ''d''. #define KERN_EMERG KERN_SOH "0" /* system is unusable */ #define KERN_ALERT KERN_SOH "1" /* action must be taken immediately */ etc...> And if that changes the log-level to "2" instead of the default "4"?No it doesn''t. It''s not triggering that because devkmsg_writev does prefix parsing only on the old "<n>" form.
On Wed, Jun 6, 2012 at 2:19 AM, Joe Perches <joe@perches.com> wrote:> On Wed, 2012-06-06 at 02:13 +0200, Kay Sievers wrote: >> The question is what happens if you inject your new binary two-byte >> prefix, like: >> echo -e "\x01\x02Hello" > /dev/kmsg > > It's not a 2 byte binary. > It's a leading ascii SOH and a standard ascii char > '0' ... '7' or 'd'. > > #define KERN_EMERG KERN_SOH "0" /* system is unusable */ > #define KERN_ALERT KERN_SOH "1" /* action must be taken immediately */ > etc...Ok.>> And if that changes the log-level to "2" instead of the default "4"? > > No it doesn't.So: echo -e "\x012Hello" > /dev/kmsg is still level 4? Sounds all fine then.> It's not triggering that because devkmsg_writev does > prefix parsing only on the old "<n>" form.Yeah, but printk_emit() will not try to parse it? I did not check, but with your change, the prefix parsing in printk_emit() is still skipped if a level is given as a parameter to printk_emit(), right? Kay _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
On Wed, 2012-06-06 at 02:28 +0200, Kay Sievers wrote:> On Wed, Jun 6, 2012 at 2:19 AM, Joe Perches <joe@perches.com> wrote: > > On Wed, 2012-06-06 at 02:13 +0200, Kay Sievers wrote: > >> The question is what happens if you inject your new binary two-byte > >> prefix, like: > >> echo -e "\x01\x02Hello" > /dev/kmsg > > > > It''s not a 2 byte binary. > > It''s a leading ascii SOH and a standard ascii char > > ''0'' ... ''7'' or ''d''. > > > > #define KERN_EMERG KERN_SOH "0" /* system is unusable */ > > #define KERN_ALERT KERN_SOH "1" /* action must be taken immediately */ > > etc... > > Ok. > > >> And if that changes the log-level to "2" instead of the default "4"? > > > > No it doesn''t. > > So: > echo -e "\x012Hello" > /dev/kmsg > is still level 4? Sounds all fine then.Yes. # echo -e "\x012Hello again Kay" > /dev/kmsg gives: 12,780,6031964979;Hello again Kay> > It''s not triggering that because devkmsg_writev does > > prefix parsing only on the old "<n>" form. > > Yeah, but printk_emit() will not try to parse it? I did not check, but > with your change, the prefix parsing in printk_emit() is still skipped > if a level is given as a parameter to printk_emit(), right?If level is not -1, then whatever prefix level the string has is ignored by vprintk_emit. from vprintk_emit: /* strip syslog prefix and extract log level or control flags */ kern_level = printk_get_level(text); if (kern_level) { const char *end_of_header = printk_skip_level(text); switch (kern_level) { case ''0'' ... ''7'': if (level == -1) level = kern_level - ''0''; case ''d'': /* Strip d KERN_DEFAULT, start new line */ plen = 0; default: if (!new_text_line) { log_buf_emit_char(''\n''); new_text_line = 1; } } text_len -= end_of_header - text; text = (char *)end_of_header; } Only level == -1 will use the prefix level. devkmsg_writev always passes a non -1 level. cheers, Joe
On Tue, 05 Jun 2012 17:07:27 -0700 Joe Perches <joe@perches.com> wrote:> On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote: > > echo "\0014Hello Joe" > /dev/kmsg > > # echo -e "\x014Hello Me" > /dev/kmsg > gives: > 12,778,4057982669;Hello MeThat''s changed behavior. On Wed, 6 Jun 2012 02:28:39 +0200 Kay Sievers <kay@vrfy.org> wrote:> Yeah, but printk_emit() will not try to parse it? I did not check, but > with your change, the prefix parsing in printk_emit() is still skipped > if a level is given as a parameter to printk_emit(), right?printk_emit() does parse the leading \0014, and then skips over it, removing it from the output stream. printk_emit() then throws away the resulting level because devkmsg_writev() did not pass in level==-1.
On Tue, 2012-06-05 at 17:37 -0700, Andrew Morton wrote:> On Tue, 05 Jun 2012 17:07:27 -0700 Joe Perches <joe@perches.com> wrote: > > > On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote: > > > echo "\0014Hello Joe" > /dev/kmsg > > > > # echo -e "\x014Hello Me" > /dev/kmsg > > gives: > > 12,778,4057982669;Hello Me > > That''s changed behavior.Which is an improvement too. I very much doubt a single app will change because of this.> printk_emit() does parse the leading \0014, and then skips over it, > removing it from the output stream. printk_emit() then throws away the > resulting level because devkmsg_writev() did not pass in level==-1.I''m glad you know how it works now. cheers, Joe
On Tue, 05 Jun 2012 17:40:05 -0700 Joe Perches <joe@perches.com> wrote:> On Tue, 2012-06-05 at 17:37 -0700, Andrew Morton wrote: > > On Tue, 05 Jun 2012 17:07:27 -0700 Joe Perches <joe@perches.com> wrote: > > > > > On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote: > > > > echo "\0014Hello Joe" > /dev/kmsg > > > > > > # echo -e "\x014Hello Me" > /dev/kmsg > > > gives: > > > 12,778,4057982669;Hello Me > > > > That''s changed behavior. > > Which is an improvement too.No it isn''t. It exposes internal kernel implementation details in random weird inexplicable ways. It doesn''t seem at all important though.> I very much doubt a single app will change > because of this.I doubt it as well.> > printk_emit() does parse the leading \0014, and then skips over it, > > removing it from the output stream. printk_emit() then throws away the > > resulting level because devkmsg_writev() did not pass in level==-1. > > I''m glad you know how it works now.It''s nice to see you learning about it as well.
On Wed, Jun 6, 2012 at 2:46 AM, Andrew Morton <akpm@linux-foundation.org> wrote:> On Tue, 05 Jun 2012 17:40:05 -0700 Joe Perches <joe@perches.com> wrote: > >> On Tue, 2012-06-05 at 17:37 -0700, Andrew Morton wrote: >> > On Tue, 05 Jun 2012 17:07:27 -0700 Joe Perches <joe@perches.com> wrote: >> > >> > > On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote: >> > > > echo "\0014Hello Joe" > /dev/kmsg >> > > >> > > # echo -e "\x014Hello Me" > /dev/kmsg >> > > gives: >> > > 12,778,4057982669;Hello Me >> > >> > That''s changed behavior. >> >> Which is an improvement too. > > No it isn''t. It exposes internal kernel implementation details in > random weird inexplicable ways. It doesn''t seem at all important > though. > >> I very much doubt a single app will change >> because of this. > > I doubt it as well.Yeah, the value of injecting such binary data is kind of questionable. :) Joe, maybe you can change printk_emit() to skip the prefix detection/stripping if a prefix is already passed to the function? Kay
On Wed, 2012-06-06 at 03:10 +0200, Kay Sievers wrote:> On Wed, Jun 6, 2012 at 2:46 AM, Andrew Morton <akpm@linux-foundation.org> wrote: > > On Tue, 05 Jun 2012 17:40:05 -0700 Joe Perches <joe@perches.com> wrote: > > > >> On Tue, 2012-06-05 at 17:37 -0700, Andrew Morton wrote: > >> > On Tue, 05 Jun 2012 17:07:27 -0700 Joe Perches <joe@perches.com> wrote: > >> > > >> > > On Tue, 2012-06-05 at 16:58 -0700, Andrew Morton wrote: > >> > > > echo "\0014Hello Joe" > /dev/kmsg > >> > > > >> > > # echo -e "\x014Hello Me" > /dev/kmsg > >> > > gives: > >> > > 12,778,4057982669;Hello Me > >> > > >> > That''s changed behavior. > >> > >> Which is an improvement too. > > > > No it isn''t. It exposes internal kernel implementation details in > > random weird inexplicable ways. It doesn''t seem at all important > > though. > > > >> I very much doubt a single app will change > >> because of this. > > > > I doubt it as well. > > Yeah, the value of injecting such binary data is kind of questionable. :) > > Joe, maybe you can change printk_emit() to skip the prefix > detection/stripping if a prefix is already passed to the function?Sure, it''s pretty trivial. Perhaps all binary data data should be elided. Maybe print . for all non-printable ascii chars.
Joe Perches
2012-Jun-06 03:04 UTC
[PATCH 9/8] printk: Only look for prefix levels in kernel messages
vprintk_emit prefix parsing should only be done for internal
kernel messages. This allows existing behavior to be kept
in all cases.
Signed-off-by: Joe Perches <joe@perches.com>
---
kernel/printk.c | 32 +++++++++++++++++---------------
1 files changed, 17 insertions(+), 15 deletions(-)
diff --git a/kernel/printk.c b/kernel/printk.c
index 5cd73f7..4e72c07 100644
--- a/kernel/printk.c
+++ b/kernel/printk.c
@@ -1267,7 +1267,6 @@ asmlinkage int vprintk_emit(int facility, int level,
static char cont_buf[LOG_LINE_MAX];
static size_t cont_len;
static int cont_level;
- int kern_level;
static struct task_struct *cont_task;
static char textbuf[LOG_LINE_MAX];
char *text = textbuf;
@@ -1329,21 +1328,24 @@ asmlinkage int vprintk_emit(int facility, int level,
newline = true;
}
- /* strip syslog prefix and extract log level or control flags */
- kern_level = printk_get_level(text);
- if (kern_level) {
- const char *end_of_header = printk_skip_level(text);
- switch (kern_level) {
- case ''0'' ... ''7'':
- if (level == -1)
- level = kern_level - ''0'';
- case ''d'': /* KERN_DEFAULT */
- prefix = true;
- case ''c'': /* KERN_CONT */
- break;
+ /* strip kernel syslog prefix and extract log level or control flags */
+ if (facility == 0) {
+ int kern_level = printk_get_level(text);
+
+ if (kern_level) {
+ const char *end_of_header = printk_skip_level(text);
+ switch (kern_level) {
+ case ''0'' ... ''7'':
+ if (level == -1)
+ level = kern_level - ''0'';
+ case ''d'': /* KERN_DEFAULT */
+ prefix = true;
+ case ''c'': /* KERN_CONT */
+ break;
+ }
+ text_len -= end_of_header - text;
+ text = (char *)end_of_header;
}
- text_len -= end_of_header - text;
- text = (char *)end_of_header;
}
if (level == -1)