Hello Jakub,
On Mon, Nov 18, 2024 at 06:33:36PM -0800, Jakub Kicinski wrote:
> Sorry for the late review, I think this will miss v6.13 :(
That is fine, there is no rush for this change.
> On Wed, 13 Nov 2024 07:10:53 -0800 Breno Leitao wrote:
> > /**
> > * struct netconsole_target - Represents a configured netconsole target.
> > * @list: Links this target into the target_list.
> > @@ -97,6 +105,7 @@ static struct console netconsole_ext;
> > * @userdata_group: Links to the userdata configfs hierarchy
> > * @userdata_complete: Cached, formatted string of append
> > * @userdata_length: String length of userdata_complete
> > + * @userdata_auto: Kernel auto-populated bitwise fields in userdata.
> > * @enabled: On / off knob to enable / disable target.
> > * Visible from userspace (read-write).
> > * We maintain a strict 1:1 correspondence between this and
> > @@ -123,6 +132,7 @@ struct netconsole_target {
> > struct config_group userdata_group;
> > char userdata_complete[MAX_USERDATA_ENTRY_LENGTH * MAX_USERDATA_ITEMS];
> > size_t userdata_length;
> > + enum userdata_auto userdata_auto;
>
> If you want to set multiple bits here type should probably be unsigned
> long. Otherwise the enum will contain combination of its values, which
> are in themselves not valid enum values ... if that makes sense.
Yes, it does make sense. I had the feeling that something was off as
well, but I was unclear if using something different than `enum
userdata_auto` would be better. I will change to `unsigned long`
>
> > #endif
> > bool enabled;
> > bool extended;
>
> > + /* Check if CPU NR should be populated, and append it to the user
> > + * dictionary.
> > + */
> > + if (child_count < MAX_USERDATA_ITEMS && nt->userdata_auto & AUTO_CPU_NR)
> > + scnprintf(&nt->userdata_complete[complete_idx],
> > + MAX_USERDATA_ENTRY_LENGTH, " cpu=%u\n",
> > + raw_smp_processor_id());
>
> I guess it may be tricky for backward compat, but shouldn't we return
> an error rather than silently skip?
yes, this should be easy to do, in fact. Probably return -E2BIG to
userspace when trying to update the entry. I thought about something as
the following patch, and piggy-back into it.
Author: Breno Leitao <leitao@xxxxxxxxxx>
Date: Tue Nov 19 04:32:56 2024 -0800
netconsole: Enforce userdata entry limit
Currently, attempting to add more than MAX_USERDATA_ITEMS to the userdata
dictionary silently fails. This patch modifies the code to return -E2BIG
when the number of elements exceeds the preallocated limit, providing clear
feedback to userspace about the failure.
Suggested-by: Jakub Kicinski <kuba@xxxxxxxxxx>
Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
index 4ea44a2f48f7b..41cff8c8e8f42 100644
--- a/drivers/net/netconsole.c
+++ b/drivers/net/netconsole.c
@@ -692,10 +692,11 @@ static ssize_t userdatum_value_show(struct config_item *item, char *buf)
return sysfs_emit(buf, "%s\n", &(to_userdatum(item)->value[0]));
}
-static void update_userdata(struct netconsole_target *nt)
+static int update_userdata(struct netconsole_target *nt)
{
int complete_idx = 0, child_count = 0;
struct list_head *entry;
+ int ret = 0;
/* Clear the current string in case the last userdatum was deleted */
nt->userdata_length = 0;
@@ -705,8 +706,10 @@ static void update_userdata(struct netconsole_target *nt)
struct userdatum *udm_item;
struct config_item *item;
- if (child_count >= MAX_USERDATA_ITEMS)
+ if (child_count >= MAX_USERDATA_ITEMS) {
+ ret = -E2BIG;
break;
+ }
child_count++;
item = container_of(entry, struct config_item, ci_entry);
@@ -726,6 +729,7 @@ static void update_userdata(struct netconsole_target *nt)
}
nt->userdata_length = strnlen(nt->userdata_complete,
sizeof(nt->userdata_complete));
+ return ret;
}
static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
@@ -748,8 +752,9 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
ud = to_userdata(item->ci_parent);
nt = userdata_to_target(ud);
- update_userdata(nt);
- ret = count;
+ ret = update_userdata(nt);
+ if (!ret)
+ ret = count;
out_unlock:
mutex_unlock(&dynamic_netconsole_mutex);
return ret;
>
> > nt->userdata_length = strnlen(nt->userdata_complete,
> > sizeof(nt->userdata_complete));
> > }
> > @@ -757,7 +788,36 @@ static ssize_t userdatum_value_store(struct config_item *item, const char *buf,
> > return ret;
> > }
> >
> > +static ssize_t populate_cpu_nr_store(struct config_item *item, const char *buf,
> > + size_t count)
> > +{
> > + struct netconsole_target *nt = to_target(item->ci_parent);
> > + bool cpu_nr_enabled;
> > + ssize_t ret;
> > +
> > + if (!nt)
> > + return -EINVAL;
>
> Can this happen? Only if function gets called with a NULL @item
> which would be pretty nutty.
Probably not. It is just me being chicken here. I will remove it for the
next version.
Thanks for the review,
--breno
