| 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
| ||
|
Message-ID: <1b5db863-7949-e4a2-7f22-6dd79ecb8089@cumulusnetworks.com>
Date: Sat, 24 Mar 2018 09:02:45 -0600
From: David Ahern <dsa@...ulusnetworks.com>
To: Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net,
roopa@...ulusnetworks.com, shm@...ulusnetworks.com,
jiri@...lanox.com, idosch@...lanox.com,
David Ahern <dsahern@...il.com>
Subject: Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource
controller via devlink
On 3/23/18 9:47 PM, Jakub Kicinski wrote:
> On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:
>> From: David Ahern <dsahern@...il.com>
>>
>> Add devlink support to netdevsim and use it to implement a simple,
>> profile based resource controller. Only one controller is needed
>> per namespace, so the first netdevsim netdevice in a namespace
>> registers with devlink. If that device is deleted, the resource
>> settings are deleted.
>
> FWIW some nits from me blow.
>
>> diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>> index 09388c06171d..449b2a1a1800 100644
>> --- a/drivers/net/netdevsim/Makefile
>> +++ b/drivers/net/netdevsim/Makefile
>> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y)
>> netdevsim-objs += \
>> bpf.o
>> endif
>> +
>> +ifneq ($(CONFIG_NET_DEVLINK),)
>
> Hm. Don't you need MAY_USE_DEVLINK dependency perhaps?
mlxsw uses CONFIG_NET_DEVLINK in its Makefile.
MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me
why it is needed at all.
>
>> +netdevsim-objs += devlink.o fib.o
>> +endif
>> diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c
>> new file mode 100644
>> index 000000000000..d10558e1b022
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/devlink.c
>
>> +static int devlink_resources_register(struct devlink *devlink)
>> +{
>> + struct devlink_resource_size_params params = {
>> + .size_max = (u64)-1,
>> + .size_granularity = 1,
>> + .unit = DEVLINK_RESOURCE_UNIT_ENTRY
>> + };
>> + struct net *net = devlink_net(devlink);
>> + int err;
>> + u64 n;
>> +
>> + /* Resources for IPv4 */
>> + err = devlink_resource_register(devlink, "IPv4", (u64)-1,
>> + NSIM_RESOURCE_IPV4,
>> + DEVLINK_RESOURCE_ID_PARENT_TOP,
>> + ¶ms, NULL);
>> + if (err) {
>> + pr_err("Failed to register IPv4 top resource\n");
>> + goto out;
>
> nit: why goto out here and return err everywhere else? If I was to
> choose I'd rather see returns. goto X; X: return; is less
> obviously correct IMHO. Besides labels should be called by the
> action they perform/undo, so goto err_return? :S
Will fix. Just got lost in the many iterations leading up to the RFC.
>
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
>> + err = devlink_resource_register(devlink, "fib", n,
>> + NSIM_RESOURCE_IPV4_FIB,
>> + NSIM_RESOURCE_IPV4,
>> + ¶ms, &nsim_ipv4_fib_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv4 FIB resource\n");
>> + return err;
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
>> + err = devlink_resource_register(devlink, "fib-rules", n,
>> + NSIM_RESOURCE_IPV4_FIB_RULES,
>> + NSIM_RESOURCE_IPV4,
>> + ¶ms, &nsim_ipv4_fib_rules_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv4 FIB rules resource\n");
>> + return err;
>> + }
>> +
>> + /* Resources for IPv6 */
>> + err = devlink_resource_register(devlink, "IPv6", (u64)-1,
>> + NSIM_RESOURCE_IPV6,
>> + DEVLINK_RESOURCE_ID_PARENT_TOP,
>> + ¶ms, NULL);
>> + if (err) {
>> + pr_err("Failed to register IPv6 top resource\n");
>> + goto out;
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
>> + err = devlink_resource_register(devlink, "fib", n,
>> + NSIM_RESOURCE_IPV6_FIB,
>> + NSIM_RESOURCE_IPV6,
>> + ¶ms, &nsim_ipv6_fib_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv6 FIB resource\n");
>> + return err;
>> + }
>> +
>> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
>> + err = devlink_resource_register(devlink, "fib-rules", n,
>> + NSIM_RESOURCE_IPV6_FIB_RULES,
>> + NSIM_RESOURCE_IPV6,
>> + ¶ms, &nsim_ipv6_fib_rules_res_ops);
>> + if (err) {
>> + pr_err("Failed to register IPv6 FIB rules resource\n");
>> + return err;
>> + }
>> +out:
>> + return err;
>> +}
>
>> +void nsim_devlink_teardown(struct netdevsim *ns)
>> +{
>> + if (ns->devlink) {
>> + struct net *net = dev_net(ns->netdev);
>> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
>
> nit: reverse xmas tree
reg_devlink uses net, so the order can not be changed
>
>> + devlink_unregister(ns->devlink);
>> + devlink_free(ns->devlink);
>> + ns->devlink = NULL;
>> +
>> + nsim_devlink_net_reset(net);
>> + *reg_devlink = true;
>> + }
>> +}
>
>> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
>> new file mode 100644
>> index 000000000000..b77dcafc7158
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/fib.c
>
>> +static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event,
>> + void *ptr)
>> +{
>> + struct fib_notifier_info *info = ptr;
>> + int err;
>> +
>> + switch (event) {
>> + case FIB_EVENT_RULE_ADD: /* fall through */
>
> nit: I don't think fall through comment is needed for back-to-back
> cases.
ok.
Thanks for the review.
>
>> + case FIB_EVENT_RULE_DEL:
>> + err = nsim_fib_rule_event(info, event == FIB_EVENT_RULE_ADD);
>> + break;
>> +
>> + case FIB_EVENT_ENTRY_ADD: /* fall through */
>> + case FIB_EVENT_ENTRY_DEL:
>> + err = nsim_fib_event(info, event == FIB_EVENT_ENTRY_ADD);
>> + break;
>> + }
>> +
>> + return notifier_from_errno(err);
>> +}
Powered by blists - more mailing lists