Re: [PATCH printk 2/3] printk: move dictionary keys to dev_printk_info
On Fri 2020-09-18 14:32:41, Rasmus Villemoes wrote:
> On 18/09/2020 14.13, Petr Mladek wrote:
> > On Fri 2020-09-18 08:16:37, Rasmus Villemoes wrote:
> >> On 17/09/2020 15.16, John Ogness wrote:
> >>
> >>> if (dev->class)
> >>> subsys = dev->class->name;
> >>> else if (dev->bus)
> >>> subsys = dev->bus->name;
> >>> else
> >>> - return 0;
> >>> + return;
> >>>
> >>> - pos += snprintf(hdr + pos, hdrlen - pos, "SUBSYSTEM=%s", subsys);
> >>> - if (pos >= hdrlen)
> >>> - goto overflow;
> >>> + snprintf(dev_info->subsystem, sizeof(dev_info->subsystem), subsys);
> >>
> >> It's unlikely that subsys would contain a %, but this will be yet
> >> another place to spend brain cycles ignoring if doing static analysis.
> >> So can we not do this. Either of strXcpy() for X=s,l will do the same
> >> thing, and likely faster.
> >
> > Good point! Better be on the safe size in a generic printk() API.
> >
> > Well, I am afraid that this would be only small drop in a huge lake.
> > class->name and bus->name seems to be passed to %s in so many
> > *print*() calls all over the kernel code.
>
> So what? printf("%s", some_string_that_might_contain_percent_chars) is
> not a problem.
Grr, shame on me. I have completely messed this. The combination of
Friday afternoon and noisy kids did not help me much to get it right.
> printf(some_string_that_might_contain_percent_chars) is.
I fully agree that passing unknown string as "fmt" is dangerous and
must be used carefully. It is not needed here.
> And yes, one could do
>
> snprintf(dev_info->subsystem, sizeof(dev_info->subsystem), "%s", subsys);
>
> but one might as well avoid the snprintf overhead and use one of the
> strX functions that have the exact same semantics (copy as much as
> there's room for, ensure nul-termination).
Yes, we should use either snprinf() with %s or strXcpy().
Best Regards,
Petr
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH printk 2/3] printk: move dictionary keys to dev_printk_info
On 18/09/2020 14.13, Petr Mladek wrote:
> On Fri 2020-09-18 08:16:37, Rasmus Villemoes wrote:
>> On 17/09/2020 15.16, John Ogness wrote:
>>
>>> if (dev->class)
>>> subsys = dev->class->name;
>>> else if (dev->bus)
>>> subsys = dev->bus->name;
>>> else
>>> - return 0;
>>> + return;
>>>
>>> - pos += snprintf(hdr + pos, hdrlen - pos, "SUBSYSTEM=%s", subsys);
>>> - if (pos >= hdrlen)
>>> - goto overflow;
>>> + snprintf(dev_info->subsystem, sizeof(dev_info->subsystem), subsys);
>>
>> It's unlikely that subsys would contain a %, but this will be yet
>> another place to spend brain cycles ignoring if doing static analysis.
>> So can we not do this. Either of strXcpy() for X=s,l will do the same
>> thing, and likely faster.
>
> Good point! Better be on the safe size in a generic printk() API.
>
> Well, I am afraid that this would be only small drop in a huge lake.
> class->name and bus->name seems to be passed to %s in so many
> *print*() calls all over the kernel code.
So what? printf("%s", some_string_that_might_contain_percent_chars) is
not a problem.
printf(some_string_that_might_contain_percent_chars) is.
And yes, one could do
snprintf(dev_info->subsystem, sizeof(dev_info->subsystem), "%s", subsys);
but one might as well avoid the snprintf overhead and use one of the
strX functions that have the exact same semantics (copy as much as
there's room for, ensure nul-termination).
Rasmus
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH printk 2/3] printk: move dictionary keys to dev_printk_info
On Fri 2020-09-18 08:16:37, Rasmus Villemoes wrote: > On 17/09/2020 15.16, John Ogness wrote: > > > if (dev->class) > > subsys = dev->class->name; > > else if (dev->bus) > > subsys = dev->bus->name; > > else > > - return 0; > > + return; > > > > - pos += snprintf(hdr + pos, hdrlen - pos, "SUBSYSTEM=%s", subsys); > > - if (pos >= hdrlen) > > - goto overflow; > > + snprintf(dev_info->subsystem, sizeof(dev_info->subsystem), subsys); > > It's unlikely that subsys would contain a %, but this will be yet > another place to spend brain cycles ignoring if doing static analysis. > So can we not do this. Either of strXcpy() for X=s,l will do the same > thing, and likely faster. Good point! Better be on the safe size in a generic printk() API. Well, I am afraid that this would be only small drop in a huge lake. class->name and bus->name seems to be passed to %s in so many *print*() calls all over the kernel code. IMHO, this is not the right place to prevent the problem. Dangerous names must be prevented when a new bus, class, device is added. Best Rergards, Petr ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH printk 2/3] printk: move dictionary keys to dev_printk_info
On Thu 2020-09-17 15:22:43, John Ogness wrote: > Dictionaries are only used for SUBSYSTEM and DEVICE properties. The > current implementation stores the property names each time they are > used. This requires more space than otherwise necessary. Also, > because the dictionary entries are currently considered optional, > it cannot be relied upon that they are always available, even if the > writer wanted to store them. These issues will increase should new > dictionary properties be introduced. > > Rather than storing the subsystem and device properties in the > dict ring, introduce a struct dev_printk_info with separate fields > to store only the property values. Embed this struct within the > struct printk_info to provide guaranteed availability. > > Signed-off-by: John Ogness Reviewed-by: Petr Mladek Best Regards, Petr ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH printk 2/3] printk: move dictionary keys to dev_printk_info
On 17/09/2020 15.16, John Ogness wrote: > if (dev->class) > subsys = dev->class->name; > else if (dev->bus) > subsys = dev->bus->name; > else > - return 0; > + return; > > - pos += snprintf(hdr + pos, hdrlen - pos, "SUBSYSTEM=%s", subsys); > - if (pos >= hdrlen) > - goto overflow; > + snprintf(dev_info->subsystem, sizeof(dev_info->subsystem), subsys); It's unlikely that subsys would contain a %, but this will be yet another place to spend brain cycles ignoring if doing static analysis. So can we not do this. Either of strXcpy() for X=s,l will do the same thing, and likely faster. Rasmus ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
