| Message ID | 20241206-of_core_fix-v1-5-dc28ed56bec3@quicinc.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | of: fix bugs and improve codes | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 12 lines checked |
| robh/patch-applied | fail | build log |
On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote: > > From: Zijun Hu <quic_zijuhu@quicinc.com> > > of_device_uevent_modalias() saves MODALIAS value from offset > (@env->buflen - 1), so the available buffer size should be > (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong > size (sizeof(@env->buf) - @env->buflen). > > Fix by using right size (sizeof(@env->buf) - @env->buflen + 1). Just writing what the diff says already is not that useful. The key part you need to know is why we back up by 1 character to begin with. > > Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent") > Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > --- > drivers/of/device.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/of/device.c b/drivers/of/device.c > index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644 > --- a/drivers/of/device.c > +++ b/drivers/of/device.c > @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env * > return -ENOMEM; > > sl = of_modalias(dev->of_node, &env->buf[env->buflen-1], This could use a comment why we back up by 1. Better to put it in a variable than add/subtract 1 everywhere: /* After add_uevent_event(), buflen is at character after the nul char which needs to be overwritten */ buflen = env->buflen - 1; And then use 'buflen' throughout. > - sizeof(env->buf) - env->buflen); > + sizeof(env->buf) - env->buflen + 1); > if (sl < 0) > return sl; > - if (sl >= (sizeof(env->buf) - env->buflen)) > + if (sl >= (sizeof(env->buf) - env->buflen + 1)) > return -ENOMEM; There's another potential problem. If we return before here, we end up with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str". Maybe that doesn't matter? I haven't looked at the caller. I think a better solution to all this would be making add_uevent_var work to construct the full value. We could add "%pOFm" format that calls of_mod_alias(). Then this function becomes just: return add_uevent_var(env, "MODALIAS=%pOFm"); And of_device_modalias() can be: sl = snprintf(str, len, "%pOFm\n", dev->of_node); if (sl >= len) return -ENOMEM; return sl; Rob
On 2024/12/10 04:34, Rob Herring wrote: > On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote: >> >> From: Zijun Hu <quic_zijuhu@quicinc.com> >> >> of_device_uevent_modalias() saves MODALIAS value from offset >> (@env->buflen - 1), so the available buffer size should be >> (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong >> size (sizeof(@env->buf) - @env->buflen). >> >> Fix by using right size (sizeof(@env->buf) - @env->buflen + 1). > > Just writing what the diff says already is not that useful. The key > part you need to know is why we back up by 1 character to begin with. > will correct commit message in v2. >> >> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent") >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> >> --- >> drivers/of/device.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/of/device.c b/drivers/of/device.c >> index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644 >> --- a/drivers/of/device.c >> +++ b/drivers/of/device.c >> @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env * >> return -ENOMEM; >> >> sl = of_modalias(dev->of_node, &env->buf[env->buflen-1], > > This could use a comment why we back up by 1. Better to put it in a > variable than add/subtract 1 everywhere: > > /* After add_uevent_event(), buflen is at character after the nul char > which needs to be overwritten */ > buflen = env->buflen - 1; > > And then use 'buflen' throughout. > good proposal. may use it for v2 after discussion done. >> - sizeof(env->buf) - env->buflen); >> + sizeof(env->buf) - env->buflen + 1); >> if (sl < 0) >> return sl; >> - if (sl >= (sizeof(env->buf) - env->buflen)) >> + if (sl >= (sizeof(env->buf) - env->buflen + 1)) >> return -ENOMEM; > > There's another potential problem. If we return before here, we end up > with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str". > Maybe that doesn't matter? I haven't looked at the caller. > that does not matter since current logic follows below 2 rules 1) all strings in @env->buf always terminated with '\0'. 2) both env->buflen and env->envp_idx are not updated once @env->buf does not enough spaces then failed. current logic has no difference with normal add_uevent_var() usage. > I think a better solution to all this would be making add_uevent_var > work to construct the full value. We could add "%pOFm" format that > calls of_mod_alias(). Then this function becomes just: > of_modalias() ? agree with you. for good practice, users should only use uevent APIs and should not depend on uevent's internal implementation. > return add_uevent_var(env, "MODALIAS=%pOFm"); > > And of_device_modalias() can be: > > sl = snprintf(str, len, "%pOFm\n", dev->of_node); > if (sl >= len) > return -ENOMEM; > return sl; > looks good. of_request_module() provides another solution. > Rob
On Tue, Dec 10, 2024 at 6:39 AM Zijun Hu <zijun_hu@icloud.com> wrote: > > On 2024/12/10 04:34, Rob Herring wrote: > > On Thu, Dec 5, 2024 at 6:53 PM Zijun Hu <zijun_hu@icloud.com> wrote: > >> > >> From: Zijun Hu <quic_zijuhu@quicinc.com> > >> > >> of_device_uevent_modalias() saves MODALIAS value from offset > >> (@env->buflen - 1), so the available buffer size should be > >> (sizeof(@env->buf) - @env->buflen + 1), but it uses the wrong > >> size (sizeof(@env->buf) - @env->buflen). > >> > >> Fix by using right size (sizeof(@env->buf) - @env->buflen + 1). > > > > Just writing what the diff says already is not that useful. The key > > part you need to know is why we back up by 1 character to begin with. > > > > will correct commit message in v2. > > >> > >> Fixes: dd27dcda37f0 ("of/device: merge of_device_uevent") > >> Signed-off-by: Zijun Hu <quic_zijuhu@quicinc.com> > >> --- > >> drivers/of/device.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/of/device.c b/drivers/of/device.c > >> index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644 > >> --- a/drivers/of/device.c > >> +++ b/drivers/of/device.c > >> @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env * > >> return -ENOMEM; > >> > >> sl = of_modalias(dev->of_node, &env->buf[env->buflen-1], > > > > This could use a comment why we back up by 1. Better to put it in a > > variable than add/subtract 1 everywhere: > > > > /* After add_uevent_event(), buflen is at character after the nul char > > which needs to be overwritten */ > > buflen = env->buflen - 1; > > > > And then use 'buflen' throughout. > > > > good proposal. may use it for v2 after discussion done. > > >> - sizeof(env->buf) - env->buflen); > >> + sizeof(env->buf) - env->buflen + 1); > >> if (sl < 0) > >> return sl; > >> - if (sl >= (sizeof(env->buf) - env->buflen)) > >> + if (sl >= (sizeof(env->buf) - env->buflen + 1)) > >> return -ENOMEM; > > > > There's another potential problem. If we return before here, we end up > > with "OF_MODALIAS=\0" or "OF_MODALIAS=some-non-nul-terminated-str". > > Maybe that doesn't matter? I haven't looked at the caller. > > > > that does not matter since current logic follows below 2 rules > > 1) all strings in @env->buf always terminated with '\0'. Ah, right. However, we still end up with a truncated value though it is nul terminated. > 2) both env->buflen and env->envp_idx are not updated once @env->buf > does not enough spaces then failed. > > current logic has no difference with normal add_uevent_var() usage. There is one major difference. add_uevent_var() will not output anything if the whole string doesn't fit. Whereas we might output a truncated value because the add_uevent_var() call updated env->buflen and env->envp_idx. We could unwind that I suppose, but that involves even more mucking with the internals of the env struct. > > I think a better solution to all this would be making add_uevent_var > > work to construct the full value. We could add "%pOFm" format that > > calls of_mod_alias(). Then this function becomes just: > > > of_modalias() ? Yes. > > agree with you. > for good practice, users should only use uevent APIs and should not > depend on uevent's internal implementation. > > > return add_uevent_var(env, "MODALIAS=%pOFm"); > > > > And of_device_modalias() can be: > > > > sl = snprintf(str, len, "%pOFm\n", dev->of_node); > > if (sl >= len) > > return -ENOMEM; > > return sl; > > > > looks good. > > of_request_module() provides another solution. Yes, that can be simplified to just a kasnprintf() call. Rob
On 2024/12/10 22:10, Rob Herring wrote: > Ah, right. However, we still end up with a truncated value though it > is nul terminated. > >> 2) both env->buflen and env->envp_idx are not updated once @env->buf >> does not enough spaces then failed. >> >> current logic has no difference with normal add_uevent_var() usage. > There is one major difference. add_uevent_var() will not output > anything if the whole string doesn't fit. Whereas we might output a > truncated value because the add_uevent_var() call updated env->buflen > and env->envp_idx. We could unwind that I suppose, but that involves > even more mucking with the internals of the env struct. you are right. i would like to try to solve involved issue in next revision. (^^)
diff --git a/drivers/of/device.c b/drivers/of/device.c index edf3be1972658f6dc165f577da53b10c7eebc116..ee29c07c83b9e6abd9b1c7747dd341026bc79eb0 100644 --- a/drivers/of/device.c +++ b/drivers/of/device.c @@ -266,10 +266,10 @@ int of_device_uevent_modalias(const struct device *dev, struct kobj_uevent_env * return -ENOMEM; sl = of_modalias(dev->of_node, &env->buf[env->buflen-1], - sizeof(env->buf) - env->buflen); + sizeof(env->buf) - env->buflen + 1); if (sl < 0) return sl; - if (sl >= (sizeof(env->buf) - env->buflen)) + if (sl >= (sizeof(env->buf) - env->buflen + 1)) return -ENOMEM; env->buflen += sl;