| Message ID | 20230206172754.980062-5-wintera@linux.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | s390/net: updates 2023-02-06 | expand |
On Mon, Feb 06, 2023 at 06:27:54PM +0100, Alexandra Winter wrote: > From: Thorsten Winkler <twinkler@linux.ibm.com> > > This LWN article explains the rationale for this change > https: //lwn.net/Articles/69419/ https://lwn.net/Articles/69419/ > Ie. snprintf() returns what *would* be the resulting length, > while scnprintf() returns the actual length. Ok, but in most cases in this patch the return value is not checked. Is there any value in this change in those cases? > Reported-by: Jules Irenge <jbi.octave@gmail.com> > Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com> s/Winkler/Winter/ ? > Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com> > Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> ... > diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c > index 1cf4e354693f..af4e60d2917e 100644 > --- a/drivers/s390/net/qeth_l3_main.c > +++ b/drivers/s390/net/qeth_l3_main.c > @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr, > char *buf) > { > if (proto == QETH_PROT_IPV4) > - return sprintf(buf, "%pI4", addr); > + return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr); > else > - return sprintf(buf, "%pI6", addr); > + return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr); > } This seems to be the once case where the return value is not ignored. Of the 4 callers of qeth_l3_ipaddr_to_string, two don't ignore the return value. And I agree in those cases this change seems correct. However, amongst other usages of the return value, those callers also check for a return < 0 from this function. Can that occur, in the sprintf or scnprintf case?
From: Simon Horman > Sent: 07 February 2023 15:43 ... > However, amongst other usages of the return value, > those callers also check for a return < 0 from this function. > Can that occur, in the sprintf or scnprintf case? That rather depends on what happens with calls like: snprintf(NULL, 0, "*%s%*s", MAX_INT, "", MAX_INT, ""); That is a whole bag of worms you don't want to put your hand into. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Wed, Feb 08, 2023 at 02:37:32PM +0000, David Laight wrote: > [You don't often get email from david.laight@aculab.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > > From: Simon Horman > > Sent: 07 February 2023 15:43 > ... > > However, amongst other usages of the return value, > > those callers also check for a return < 0 from this function. > > Can that occur, in the sprintf or scnprintf case? > > That rather depends on what happens with calls like: > snprintf(NULL, 0, "*%s%*s", MAX_INT, "", MAX_INT, ""); > > That is a whole bag of worms you don't want to put your hand into. Ok :)
On 07.02.23 16:42, Simon Horman wrote: > On Mon, Feb 06, 2023 at 06:27:54PM +0100, Alexandra Winter wrote: >> From: Thorsten Winkler <twinkler@linux.ibm.com> >> >> This LWN article explains the rationale for this change >> https: //lwn.net/Articles/69419/ > > https://lwn.net/Articles/69419/ > >> Ie. snprintf() returns what *would* be the resulting length, >> while scnprintf() returns the actual length. > > Ok, but in most cases in this patch the return value is not checked. > Is there any value in this change in those cases? > Jules Irenge reported a coccinnelle warning to use scnprintf in show() functions [1]. (Thorsten Winkler changed these instances to sysfs_emit in patch 3 of this series.) We read the article as a call to implement the plan to upgrade the kernel to the newer *scnprintf functions. Is that not intended? I totally agree, that in these cases no real problem was fixed, it is more of a style improvement. [1] https://lore.kernel.org/netdev/YzHyniCyf+G%2F2xI8@fedora/T/ >> Reported-by: Jules Irenge <jbi.octave@gmail.com> >> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com> > > s/Winkler/Winter/ ? Of course. Wow, you have good eyes! > >> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com> >> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > > ... > >> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c >> index 1cf4e354693f..af4e60d2917e 100644 >> --- a/drivers/s390/net/qeth_l3_main.c >> +++ b/drivers/s390/net/qeth_l3_main.c >> @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr, >> char *buf) >> { >> if (proto == QETH_PROT_IPV4) >> - return sprintf(buf, "%pI4", addr); >> + return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr); >> else >> - return sprintf(buf, "%pI6", addr); >> + return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr); >> } > > > This seems to be the once case where the return value is not ignored. > > Of the 4 callers of qeth_l3_ipaddr_to_string, two don't ignore the return > value. And I agree in those cases this change seems correct. > > However, amongst other usages of the return value, > those callers also check for a return < 0 from this function. > Can that occur, in the sprintf or scnprintf case? I was under the impression this was a safeguard against a bad address format, but I tried it out and it never resulted in a negative return. Thanks a lot for pointing this out, we can further simplify patch 3 with that.
On Wed, Feb 08, 2023 at 07:19:29PM +0100, Alexandra Winter wrote: > > > On 07.02.23 16:42, Simon Horman wrote: > > On Mon, Feb 06, 2023 at 06:27:54PM +0100, Alexandra Winter wrote: > >> From: Thorsten Winkler <twinkler@linux.ibm.com> > >> > >> This LWN article explains the rationale for this change > >> https: //lwn.net/Articles/69419/ > > > > https://lwn.net/Articles/69419/ > > > >> Ie. snprintf() returns what *would* be the resulting length, > >> while scnprintf() returns the actual length. > > > > Ok, but in most cases in this patch the return value is not checked. > > Is there any value in this change in those cases? > > > > Jules Irenge reported a coccinnelle warning to use scnprintf in > show() functions [1]. (Thorsten Winkler changed these instances to > sysfs_emit in patch 3 of this series.) > We read the article as a call to implement the plan to upgrade the kernel > to the newer *scnprintf functions. Is that not intended? > > I totally agree, that in these cases no real problem was fixed, it is > more of a style improvement. My feeling is that it isn't an improvement and therefore probably best not done. But that is just my opinion. > [1] https://lore.kernel.org/netdev/YzHyniCyf+G%2F2xI8@fedora/T/ > > >> Reported-by: Jules Irenge <jbi.octave@gmail.com> > >> Reviewed-by: Alexandra Winkler <wintera@linux.ibm.com> > > > > s/Winkler/Winter/ ? > > Of course. Wow, you have good eyes! Only on my good days. > >> Signed-off-by: Thorsten Winkler <twinkler@linux.ibm.com> > >> Signed-off-by: Alexandra Winter <wintera@linux.ibm.com> > > > > ... > > > >> diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c > >> index 1cf4e354693f..af4e60d2917e 100644 > >> --- a/drivers/s390/net/qeth_l3_main.c > >> +++ b/drivers/s390/net/qeth_l3_main.c > >> @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr, > >> char *buf) > >> { > >> if (proto == QETH_PROT_IPV4) > >> - return sprintf(buf, "%pI4", addr); > >> + return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr); > >> else > >> - return sprintf(buf, "%pI6", addr); > >> + return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr); > >> } > > > > > > This seems to be the once case where the return value is not ignored. > > > > Of the 4 callers of qeth_l3_ipaddr_to_string, two don't ignore the return > > value. And I agree in those cases this change seems correct. > > > > However, amongst other usages of the return value, > > those callers also check for a return < 0 from this function. > > Can that occur, in the sprintf or scnprintf case? > > I was under the impression this was a safeguard against a bad address format, > but I tried it out and it never resulted in a negative return. > Thanks a lot for pointing this out, we can further simplify patch 3 with that. The advice elsewhere in this thread is that perhaps leaving this as-is may be best after all. * https://lore.kernel.org/netdev/63c6825fc2c94ad19ac7de93a6f151f6@AcuMS.aculab.com/
diff --git a/drivers/s390/net/qeth_core_main.c b/drivers/s390/net/qeth_core_main.c index 8bd9fd51208c..1d5b207c2b9e 100644 --- a/drivers/s390/net/qeth_core_main.c +++ b/drivers/s390/net/qeth_core_main.c @@ -2801,9 +2801,11 @@ static void qeth_print_status_message(struct qeth_card *card) * of the level OSA sets the first character to zero * */ if (!card->info.mcl_level[0]) { - sprintf(card->info.mcl_level, "%02x%02x", - card->info.mcl_level[2], - card->info.mcl_level[3]); + scnprintf(card->info.mcl_level, + sizeof(card->info.mcl_level), + "%02x%02x", + card->info.mcl_level[2], + card->info.mcl_level[3]); break; } fallthrough; @@ -6090,7 +6092,7 @@ void qeth_dbf_longtext(debug_info_t *id, int level, char *fmt, ...) if (!debug_level_enabled(id, level)) return; va_start(args, fmt); - vsnprintf(dbf_txt_buf, sizeof(dbf_txt_buf), fmt, args); + vscnprintf(dbf_txt_buf, sizeof(dbf_txt_buf), fmt, args); va_end(args); debug_text_event(id, level, dbf_txt_buf); } @@ -6330,8 +6332,8 @@ static int qeth_core_probe_device(struct ccwgroup_device *gdev) goto err_dev; } - snprintf(dbf_name, sizeof(dbf_name), "qeth_card_%s", - dev_name(&gdev->dev)); + scnprintf(dbf_name, sizeof(dbf_name), "qeth_card_%s", + dev_name(&gdev->dev)); card->debug = qeth_get_dbf_entry(dbf_name); if (!card->debug) { rc = qeth_add_dbf_entry(card, dbf_name); diff --git a/drivers/s390/net/qeth_ethtool.c b/drivers/s390/net/qeth_ethtool.c index e250f49535fa..c1caf7734c3e 100644 --- a/drivers/s390/net/qeth_ethtool.c +++ b/drivers/s390/net/qeth_ethtool.c @@ -172,7 +172,7 @@ static void qeth_get_strings(struct net_device *dev, u32 stringset, u8 *data) qeth_add_stat_strings(&data, prefix, card_stats, CARD_STATS_LEN); for (i = 0; i < card->qdio.no_out_queues; i++) { - snprintf(prefix, ETH_GSTRING_LEN, "tx%u ", i); + scnprintf(prefix, ETH_GSTRING_LEN, "tx%u ", i); qeth_add_stat_strings(&data, prefix, txq_stats, TXQ_STATS_LEN); } @@ -192,8 +192,8 @@ static void qeth_get_drvinfo(struct net_device *dev, sizeof(info->driver)); strscpy(info->fw_version, card->info.mcl_level, sizeof(info->fw_version)); - snprintf(info->bus_info, sizeof(info->bus_info), "%s/%s/%s", - CARD_RDEV_ID(card), CARD_WDEV_ID(card), CARD_DDEV_ID(card)); + scnprintf(info->bus_info, sizeof(info->bus_info), "%s/%s/%s", + CARD_RDEV_ID(card), CARD_WDEV_ID(card), CARD_DDEV_ID(card)); } static void qeth_get_channels(struct net_device *dev, diff --git a/drivers/s390/net/qeth_l2_main.c b/drivers/s390/net/qeth_l2_main.c index c6ded3fdd715..9f13ed170a43 100644 --- a/drivers/s390/net/qeth_l2_main.c +++ b/drivers/s390/net/qeth_l2_main.c @@ -1255,37 +1255,38 @@ static void qeth_bridge_emit_host_event(struct qeth_card *card, switch (evtype) { case anev_reg_unreg: - snprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=%s", - (code & IPA_ADDR_CHANGE_CODE_REMOVAL) - ? "deregister" : "register"); + scnprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=%s", + (code & IPA_ADDR_CHANGE_CODE_REMOVAL) + ? "deregister" : "register"); env[i] = str[i]; i++; if (code & IPA_ADDR_CHANGE_CODE_VLANID) { - snprintf(str[i], sizeof(str[i]), "VLAN=%d", - addr_lnid->lnid); + scnprintf(str[i], sizeof(str[i]), "VLAN=%d", + addr_lnid->lnid); env[i] = str[i]; i++; } if (code & IPA_ADDR_CHANGE_CODE_MACADDR) { - snprintf(str[i], sizeof(str[i]), "MAC=%pM", - addr_lnid->mac); + scnprintf(str[i], sizeof(str[i]), "MAC=%pM", + addr_lnid->mac); env[i] = str[i]; i++; } - snprintf(str[i], sizeof(str[i]), "NTOK_BUSID=%x.%x.%04x", - token->cssid, token->ssid, token->devnum); + scnprintf(str[i], sizeof(str[i]), "NTOK_BUSID=%x.%x.%04x", + token->cssid, token->ssid, token->devnum); env[i] = str[i]; i++; - snprintf(str[i], sizeof(str[i]), "NTOK_IID=%02x", token->iid); + scnprintf(str[i], sizeof(str[i]), "NTOK_IID=%02x", token->iid); env[i] = str[i]; i++; - snprintf(str[i], sizeof(str[i]), "NTOK_CHPID=%02x", - token->chpid); + scnprintf(str[i], sizeof(str[i]), "NTOK_CHPID=%02x", + token->chpid); env[i] = str[i]; i++; - snprintf(str[i], sizeof(str[i]), "NTOK_CHID=%04x", token->chid); + scnprintf(str[i], sizeof(str[i]), "NTOK_CHID=%04x", + token->chid); env[i] = str[i]; i++; break; case anev_abort: - snprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=abort"); + scnprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=abort"); env[i] = str[i]; i++; break; case anev_reset: - snprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=reset"); + scnprintf(str[i], sizeof(str[i]), "BRIDGEDHOST=reset"); env[i] = str[i]; i++; break; } @@ -1314,17 +1315,17 @@ static void qeth_bridge_state_change_worker(struct work_struct *work) NULL }; - snprintf(env_locrem, sizeof(env_locrem), "BRIDGEPORT=statechange"); - snprintf(env_role, sizeof(env_role), "ROLE=%s", - (data->role == QETH_SBP_ROLE_NONE) ? "none" : - (data->role == QETH_SBP_ROLE_PRIMARY) ? "primary" : - (data->role == QETH_SBP_ROLE_SECONDARY) ? "secondary" : - "<INVALID>"); - snprintf(env_state, sizeof(env_state), "STATE=%s", - (data->state == QETH_SBP_STATE_INACTIVE) ? "inactive" : - (data->state == QETH_SBP_STATE_STANDBY) ? "standby" : - (data->state == QETH_SBP_STATE_ACTIVE) ? "active" : - "<INVALID>"); + scnprintf(env_locrem, sizeof(env_locrem), "BRIDGEPORT=statechange"); + scnprintf(env_role, sizeof(env_role), "ROLE=%s", + (data->role == QETH_SBP_ROLE_NONE) ? "none" : + (data->role == QETH_SBP_ROLE_PRIMARY) ? "primary" : + (data->role == QETH_SBP_ROLE_SECONDARY) ? "secondary" : + "<INVALID>"); + scnprintf(env_state, sizeof(env_state), "STATE=%s", + (data->state == QETH_SBP_STATE_INACTIVE) ? "inactive" : + (data->state == QETH_SBP_STATE_STANDBY) ? "standby" : + (data->state == QETH_SBP_STATE_ACTIVE) ? "active" : + "<INVALID>"); kobject_uevent_env(&data->card->gdev->dev.kobj, KOBJ_CHANGE, env); kfree(data); diff --git a/drivers/s390/net/qeth_l3_main.c b/drivers/s390/net/qeth_l3_main.c index 1cf4e354693f..af4e60d2917e 100644 --- a/drivers/s390/net/qeth_l3_main.c +++ b/drivers/s390/net/qeth_l3_main.c @@ -47,9 +47,9 @@ int qeth_l3_ipaddr_to_string(enum qeth_prot_versions proto, const u8 *addr, char *buf) { if (proto == QETH_PROT_IPV4) - return sprintf(buf, "%pI4", addr); + return scnprintf(buf, INET_ADDRSTRLEN, "%pI4", addr); else - return sprintf(buf, "%pI6", addr); + return scnprintf(buf, INET6_ADDRSTRLEN, "%pI6", addr); } static struct qeth_ipaddr *qeth_l3_find_addr_by_ip(struct qeth_card *card, diff --git a/drivers/s390/net/qeth_l3_sys.c b/drivers/s390/net/qeth_l3_sys.c index f0f8adaa2f05..cce6e621cd88 100644 --- a/drivers/s390/net/qeth_l3_sys.c +++ b/drivers/s390/net/qeth_l3_sys.c @@ -252,8 +252,8 @@ static ssize_t qeth_l3_dev_hsuid_store(struct device *dev, goto out; } - snprintf(card->options.hsuid, sizeof(card->options.hsuid), - "%-8s", tmp); + scnprintf(card->options.hsuid, sizeof(card->options.hsuid), + "%-8s", tmp); ASCEBC(card->options.hsuid, 8); memcpy(card->dev->perm_addr, card->options.hsuid, 9);