lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Nov 2014 16:37:23 +0100 From: Jiri Pirko <jiri@...nulli.us> To: Hamad Kadmany <hkadmany@...eaurora.org> Cc: netdev@...r.kernel.org Subject: Re: [PATCH] net: team: expose sysfs attributes for each team option Wed, Nov 19, 2014 at 01:40:43PM CET, jiri@...nulli.us wrote: >Wed, Nov 19, 2014 at 01:28:19PM CET, hkadmany@...eaurora.org wrote: >>On Mon, November 17, 2014 1:02 pm, Jiri Pirko wrote: >>> >>> Nak. >>> >>> I don't like this patch. The plan for team was keep things simple and to >>> have single userspace api using netlink, as that is the correct way to >>> deal with things. Sysfs for this use-case is not. Please, use netlink api. >> >>Using team driver requires using libnl3. In Android based platforms, libnl3 is not supported, and libnl3 license poses constraints in Android's user-space. >> >>This is the reason for the this change, to have at least the team options exposed through sysfs. > >No, that is certainly not the reason. If libnl3 does not suit you, use a >different lib. It is in my opinion unacceptaptable to work around >certain licence issues by adding parallel sysfs api (not to mention that >sysfs is totally unsuitable for that). > >You should use existing Netlink API. That's it. Also, using Netlink API, you get all the events to your userspace daemon. Sysfs is just not suitable for that. > >> >> >>> >>>> >>>>Signed-off-by: Hamad Kadmany <hkadmany@...eaurora.org> >>>>--- >>>> drivers/net/team/team.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++-- >>>> include/linux/if_team.h | 3 + >>>> 2 files changed, 278 insertions(+), 7 deletions(-) >>>> >>>>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c >>>>index 1222229..afd2f8f 100644 >>>>--- a/drivers/net/team/team.c >>>>+++ b/drivers/net/team/team.c >>>>@@ -110,8 +110,198 @@ struct team_option_inst { /* One for each option instance */ >>>> struct team_option_inst_info info; >>>> bool changed; >>>> bool removed; >>>>+ bool dev_attr_file_exist; >>>>+ struct device_attribute dev_attr; /* corresponding sysfs attribute */ >>>> }; >>>> >>>>+static struct attribute *team_sysfs_attrs[] = { >>>>+ NULL, >>>>+}; >>>>+ >>>>+static struct attribute_group team_sysfs_attr_group = { >>>>+ .attrs = team_sysfs_attrs, >>>>+}; >>>>+ >>>>+/* Device attributes (sysfs) */ >>>>+ >>>>+static ssize_t show_attribute(struct device *dev, >>>>+ struct device_attribute *attr, >>>>+ char *buf) >>>>+{ >>>>+ struct team *team = dev_get_drvdata(dev); >>>>+ struct team_option_inst *opt_inst; >>>>+ ssize_t ret; >>>>+ struct team_option *option; >>>>+ struct team_gsetter_ctx ctx; >>>>+ >>>>+ if (mutex_lock_interruptible(&team->lock)) >>>>+ return -ERESTARTSYS; >>>>+ >>>>+ opt_inst = container_of(attr, struct team_option_inst, dev_attr); >>>>+ option = opt_inst->option; >>>>+ if (!option->getter) { >>>>+ ret = -EOPNOTSUPP; >>>>+ netdev_err(team->dev, >>>>+ "Option %s is write only\n", attr->attr.name); >>>>+ goto exit; >>>>+ } >>>>+ >>>>+ ctx.info = &opt_inst->info; >>>>+ /* let the option getter do its job */ >>>>+ ret = option->getter(team, &ctx); >>>>+ if (ret) >>>>+ goto exit; >>>>+ >>>>+ /* translate option's output into sysfs output */ >>>>+ switch (option->type) { >>>>+ case TEAM_OPTION_TYPE_U32: >>>>+ ret = scnprintf(buf, PAGE_SIZE, "%u\n", ctx.data.u32_val); >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_STRING: >>>>+ ret = scnprintf(buf, PAGE_SIZE, "%s\n", ctx.data.str_val); >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_BINARY: >>>>+ if (ctx.data.bin_val.len > PAGE_SIZE) { >>>>+ netdev_err(team->dev, >>>>+ "Option output is too long (%d)\n", >>>>+ ctx.data.bin_val.len); >>>>+ break; >>>>+ } >>>>+ >>>>+ memcpy(buf, ctx.data.bin_val.ptr, ctx.data.bin_val.len); >>>>+ ret = ctx.data.bin_val.len; >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_BOOL: >>>>+ ret = scnprintf(buf, PAGE_SIZE, "%d\n", ctx.data.bool_val); >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_S32: >>>>+ ret = scnprintf(buf, PAGE_SIZE, "%d\n", ctx.data.s32_val); >>>>+ break; >>>>+ default: >>>>+ BUG(); >>>>+ } >>>>+ >>>>+exit: >>>>+ mutex_unlock(&team->lock); >>>>+ >>>>+ return ret; >>>>+} >>>>+ >>>>+static int team_nl_send_event_options_get(struct team *team, >>>>+ struct list_head *sel_opt_inst_list); >>>>+ >>>>+static ssize_t set_attribute(struct device *dev, >>>>+ struct device_attribute *attr, >>>>+ const char *buf, size_t count) >>>>+{ >>>>+ struct team_option_inst *opt_inst; >>>>+ struct team *team = dev_get_drvdata(dev); >>>>+ int err = 0; >>>>+ struct team_option *option = NULL; >>>>+ struct team_gsetter_ctx ctx; >>>>+ >>>>+ LIST_HEAD(opt_inst_list); >>>>+ >>>>+ if (mutex_lock_interruptible(&team->lock)) >>>>+ return -ERESTARTSYS; >>>>+ >>>>+ opt_inst = container_of(attr, struct team_option_inst, dev_attr); >>>>+ option = opt_inst->option; >>>>+ if (!option->setter) { >>>>+ netdev_err(team->dev, >>>>+ "Option %s is read only\n", attr->attr.name); >>>>+ err = -EOPNOTSUPP; >>>>+ goto exit; >>>>+ } >>>>+ >>>>+ ctx.info = &opt_inst->info; >>>>+ >>>>+ /* translate sysfs input into option's input */ >>>>+ switch (option->type) { >>>>+ case TEAM_OPTION_TYPE_U32: >>>>+ err = kstrtoint(buf, 0, &ctx.data.u32_val); >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_STRING: >>>>+ if (count > TEAM_STRING_MAX_LEN) { >>>>+ netdev_err(team->dev, >>>>+ "Input buffer too long (%zu)\n", count); >>>>+ err = -EINVAL; >>>>+ break; >>>>+ } >>>>+ ctx.data.str_val = buf; >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_BINARY: >>>>+ ctx.data.bin_val.len = count; >>>>+ ctx.data.bin_val.ptr = buf; >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_BOOL: >>>>+ err = strtobool(buf, &ctx.data.bool_val); >>>>+ break; >>>>+ case TEAM_OPTION_TYPE_S32: >>>>+ err = kstrtoint(buf, 0, &ctx.data.s32_val); >>>>+ break; >>>>+ default: >>>>+ BUG(); >>>>+ } >>>>+ >>>>+ if (err) { >>>>+ netdev_err(team->dev, "Failed to translate input buffer\n"); >>>>+ goto exit; >>>>+ } >>>>+ >>>>+ /* let the option setter do its job */ >>>>+ err = option->setter(team, &ctx); >>>>+ if (err) >>>>+ goto exit; >>>>+ >>>>+ /* propagate option changed event */ >>>>+ opt_inst->changed = true; >>>>+ list_add(&opt_inst->tmp_list, &opt_inst_list); >>>>+ err = team_nl_send_event_options_get(team, &opt_inst_list); >>>>+ if (err == -ESRCH) /* no listeners, not a real error */ >>>>+ err = 0; >>>>+ >>>>+exit: >>>>+ mutex_unlock(&team->lock); >>>>+ >>>>+ if (!err) >>>>+ err = count; >>>>+ return err; >>>>+} >>>>+ >>>>+/* create sysfs attribute for each option that is being registered */ >>>>+static int __team_option_add_sysfs_attr(struct team *team, >>>>+ struct team_option_inst *opt_inst, >>>>+ bool create_sysfs_file) >>>>+{ >>>>+ int err = 0; >>>>+ struct device_attribute *new_dev_attr = &opt_inst->dev_attr; >>>>+ >>>>+ new_dev_attr->attr.name = opt_inst->option->name; >>>>+ new_dev_attr->attr.mode = S_IRUGO | S_IWUSR; >>>>+ new_dev_attr->show = show_attribute; >>>>+ new_dev_attr->store = set_attribute; >>>>+ >>>>+ if (create_sysfs_file) { >>>>+ err = sysfs_create_file(&team->dev->dev.kobj, >>>>+ &new_dev_attr->attr); >>>>+ if (err) >>>>+ netdev_err(team->dev, >>>>+ "Failed to create sysfs attribute %s\n", >>>>+ new_dev_attr->attr.name); >>>>+ } >>>>+ >>>>+ return err; >>>>+} >>>>+ >>>>+static void __team_option_del_sysfs_attr(struct team *team, >>>>+ struct team_option_inst *opt_inst) >>>>+{ >>>>+ if (opt_inst->dev_attr_file_exist) >>>>+ sysfs_remove_file(&team->dev->dev.kobj, >>>>+ &opt_inst->dev_attr.attr); >>>>+} >>>>+ >>>> static struct team_option *__team_find_option(struct team *team, >>>> const char *opt_name) >>>> { >>>>@@ -124,8 +314,10 @@ static struct team_option *__team_find_option(struct team *team, >>>> return NULL; >>>> } >>>> >>>>-static void __team_option_inst_del(struct team_option_inst *opt_inst) >>>>+static void __team_option_inst_del(struct team *team, >>>>+ struct team_option_inst *opt_inst) >>>> { >>>>+ __team_option_del_sysfs_attr(team, opt_inst); >>>> list_del(&opt_inst->list); >>>> kfree(opt_inst); >>>> } >>>>@@ -137,7 +329,7 @@ static void __team_option_inst_del_option(struct team *team, >>>> >>>> list_for_each_entry_safe(opt_inst, tmp, &team->option_inst_list, list) { >>>> if (opt_inst->option == option) >>>>- __team_option_inst_del(opt_inst); >>>>+ __team_option_inst_del(team, opt_inst); >>>> } >>>> } >>>> >>>>@@ -162,6 +354,7 @@ static int __team_option_inst_add(struct team *team, struct team_option *option, >>>> opt_inst->info.array_index = i; >>>> opt_inst->changed = true; >>>> opt_inst->removed = false; >>>>+ opt_inst->dev_attr_file_exist = false; >>>> list_add_tail(&opt_inst->list, &team->option_inst_list); >>>> if (option->init) { >>>> err = option->init(team, &opt_inst->info); >>>>@@ -170,6 +363,20 @@ static int __team_option_inst_add(struct team *team, struct team_option *option, >>>> } >>>> >>>> } >>>>+ >>>>+ /* add sysfs attribute. per-port and array options are skipped */ >>>>+ if (!option->per_port && !option->array_size) { >>>>+ /* create the sysfs file only if our state allows it */ >>>>+ bool create_sysfs_file = device_is_registered(&team->dev->dev); >>>>+ >>>>+ err = __team_option_add_sysfs_attr(team, opt_inst, >>>>+ create_sysfs_file); >>>>+ if (err) >>>>+ return err; >>>>+ >>>>+ opt_inst->dev_attr_file_exist = true; >>>>+ } >>>>+ >>>> return 0; >>>> } >>>> >>>>@@ -218,7 +425,7 @@ static void __team_option_inst_del_port(struct team *team, >>>> list_for_each_entry_safe(opt_inst, tmp, &team->option_inst_list, list) { >>>> if (opt_inst->option->per_port && >>>> opt_inst->info.port == port) >>>>- __team_option_inst_del(opt_inst); >>>>+ __team_option_inst_del(team, opt_inst); >>>> } >>>> } >>>> >>>>@@ -337,6 +544,51 @@ static void __team_options_unregister(struct team *team, >>>> >>>> static void __team_options_change_check(struct team *team); >>>> >>>>+static void team_attr_grp_free(struct team *team) >>>>+{ >>>>+ kfree(team->attr_grp.attrs); >>>>+} >>>>+ >>>>+/* allocate attribute group for creating sysfs for team's own options */ >>>>+static int team_attr_grp_alloc(struct team *team) >>>>+{ >>>>+ struct attribute **attributes; >>>>+ struct team_option_inst *opt_inst; >>>>+ int num_attr = 0; >>>>+ struct team_option *option; >>>>+ >>>>+ list_for_each_entry(opt_inst, &team->option_inst_list, list) { >>>>+ option = opt_inst->option; >>>>+ /* per-port and array options currently not supported as >>>>+ * sysfs attributes >>>>+ */ >>>>+ if (option->per_port || option->array_size) >>>>+ continue; >>>>+ >>>>+ num_attr++; >>>>+ } >>>>+ >>>>+ /* +1 for having NULL as last item in the array */ >>>>+ attributes = kzalloc((num_attr + 1) * sizeof(*attributes), GFP_KERNEL); >>>>+ if (!attributes) >>>>+ return -ENOMEM; >>>>+ team->attr_grp.attrs = attributes; >>>>+ >>>>+ num_attr = 0; >>>>+ list_for_each_entry(opt_inst, &team->option_inst_list, list) { >>>>+ option = opt_inst->option; >>>>+ /* per-port and array options currently not supported as >>>>+ * sysfs attributes >>>>+ */ >>>>+ if (option->per_port || option->array_size) >>>>+ continue; >>>>+ >>>>+ attributes[num_attr++] = &opt_inst->dev_attr.attr; >>>>+ } >>>>+ >>>>+ return 0; >>>>+} >>>>+ >>>> int team_options_register(struct team *team, >>>> const struct team_option *option, >>>> size_t option_count) >>>>@@ -1380,15 +1632,28 @@ static int team_init(struct net_device *dev) >>>> >>>> INIT_LIST_HEAD(&team->option_list); >>>> INIT_LIST_HEAD(&team->option_inst_list); >>>>- err = team_options_register(team, team_options, ARRAY_SIZE(team_options)); >>>>+ >>>>+ err = team_options_register(team, team_options, >>>>+ ARRAY_SIZE(team_options)); >>>> if (err) >>>> goto err_options_register; >>>> netif_carrier_off(dev); >>>> >>>> team_set_lockdep_class(dev); >>>> >>>>+ /* store team context, to be used by Device attributes getter/setter */ >>>>+ dev_set_drvdata(&dev->dev, team); >>>>+ >>>>+ /* allocate and register sysfs attributes for team's own options */ >>>>+ err = team_attr_grp_alloc(team); >>>>+ if (err) >>>>+ goto err_grp_alloc; >>>>+ dev->sysfs_groups[0] = &team->attr_grp; >>>>+ >>>> return 0; >>>> >>>>+err_grp_alloc: >>>>+ team_options_unregister(team, team_options, ARRAY_SIZE(team_options)); >>>> err_options_register: >>>> team_queue_override_fini(team); >>>> err_team_queue_override_init: >>>>@@ -1407,9 +1672,15 @@ static void team_uninit(struct net_device *dev) >>>> list_for_each_entry_safe(port, tmp, &team->port_list, list) >>>> team_port_del(team, port->dev); >>>> >>>>+ sysfs_remove_group(&team->dev->dev.kobj, &team->attr_grp); >>>>+ team_attr_grp_free(team); >>>>+ /* set to dummy group to avoid double free by core */ >>>>+ dev->sysfs_groups[0] = &team_sysfs_attr_group; >>>>+ >>>> __team_change_mode(team, NULL); /* cleanup */ >>>> __team_options_unregister(team, team_options, ARRAY_SIZE(team_options)); >>>> team_queue_override_fini(team); >>>>+ >>>> mutex_unlock(&team->lock); >>>> } >>>> >>>>@@ -2194,9 +2465,6 @@ static int team_nl_cmd_options_get(struct sk_buff *skb, struct genl_info *info) >>>> return err; >>>> } >>>> >>>>-static int team_nl_send_event_options_get(struct team *team, >>>>- struct list_head *sel_opt_inst_list); >>>>- >>>> static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info) >>>> { >>>> struct team *team; >>>>diff --git a/include/linux/if_team.h b/include/linux/if_team.h >>>>index 25b8b15..2e9fb2a 100644 >>>>--- a/include/linux/if_team.h >>>>+++ b/include/linux/if_team.h >>>>@@ -188,6 +188,9 @@ struct team { >>>> struct list_head option_list; >>>> struct list_head option_inst_list; /* list of option instances */ >>>> >>>>+ /* attribute group for registering team's own options at init */ >>>>+ struct attribute_group attr_grp; >>>>+ >>>> const struct team_mode *mode; >>>> struct team_mode_ops ops; >>>> bool user_carrier_enabled; >>>>-- >>>>1.8.5.2 >>>>-- >>>>Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc. >>>>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, >>>>a Linux Foundation Collaborative Project >>>> >>> >> >> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists