netdev - Re: [PATCH net-next, v5] netlink: add IGMP/MLD join/leave notifications

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADXeF1Ekn1cH01GjYmnN64NaGOhY0yLz30HB720u_TFabwm33A@mail.gmail.com>
Date: Tue, 10 Dec 2024 13:32:35 +0900
From: Yuyang Huang <yuyanghuang@...gle.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>, David Ahern <dsahern@...nel.org>, 
	roopa@...ulusnetworks.com, jiri@...nulli.us, stephen@...workplumber.org, 
	jimictw@...gle.com, prohr@...gle.com, liuhangbin@...il.com, 
	nicolas.dichtel@...nd.com, andrew@...n.ch, netdev@...r.kernel.org, 
	Maciej Żenczykowski <maze@...gle.com>, 
	Lorenzo Colitti <lorenzo@...gle.com>, Patrick Ruddy <pruddy@...tta.att-mail.com>
Subject: Re: [PATCH net-next, v5] netlink: add IGMP/MLD join/leave notifications

>In order to reuse the `inet6_fill_ifmcaddr()` logic, we need  move the
>function and `struct inet6_fill_args` definition into
>`net/core/rtnetlink.c` and `include/net/rtnetlink.h`. Is this approach
>acceptable?

Instead of touching rtnetlink.{h.c}, I realized it might be more
suitable to move `inet6_fill_ifmcaddr()` and `struct inet6_fill_args`
into addrconf.h.

Thanks,
Yuyang

On Tue, Dec 10, 2024 at 12:19 PM Yuyang Huang <yuyanghuang@...gle.com> wrote:
>
> Thanks for the review feedback.
>
> >Is there a strong reason you reimplement this instead of trying to reuse
> >inet6_fill_ifmcaddr() ? Keeping notifications and get responses in sync
> >used to be a thing in rtnetlink, this code already diverged but maybe
> >we can bring it back.
>
> In order to reuse the `inet6_fill_ifmcaddr()` logic, we need  move the
> function and `struct inet6_fill_args` definition into
> `net/core/rtnetlink.c` and `include/net/rtnetlink.h`. Is this approach
> acceptable?
>
> In this patch, we will always set `ifa_scope` to `RT_SCOPE_UNIVERSE`
> for both IPv4 and IPv6 multicast addresses for consistency. Reusing
> `inet6_fill_ifmcaddr()` will revert to the following logic:
>
> >u8 scope = RT_SCOPE_UNIVERSE;
> >struct nlmsghdr *nlh;
> >if (ipv6_addr_scope(&ifmca->mca_addr) & IFA_SITE)
> scope = RT_SCOPE_SITE;
>
> Is it acceptable, or should I update the old logic to always set
> ‘RT_SCOPE_UNIVERSE’?
>
> >ENOMEM ? I could be wrong but in atomic context the memory pressure
> >can well be transient, it's not like the socket queue filled up.
>
> Will update in the next patch version.
>
> >nit: + goes to the end of previous line
>
> Will update in the next patch version.
>
> >nit: nlmsg_free(), since it exists
>
> Will update in the next patch version.
>
> Thanks,
> Yuyang
>
>
> On Tue, Dec 10, 2024 at 11:25 AM Jakub Kicinski <kuba@...nel.org> wrote:
> >
> > On Fri,  6 Dec 2024 13:10:25 +0900 Yuyang Huang wrote:
> > > +static int inet6_fill_ifmcaddr(struct sk_buff *skb, struct net_device *dev,
> > > +                            const struct in6_addr *addr, int event)
> > > +{
> > > +     struct ifaddrmsg *ifm;
> > > +     struct nlmsghdr *nlh;
> > > +
> > > +     nlh = nlmsg_put(skb, 0, 0, event, sizeof(struct ifaddrmsg), 0);
> > > +     if (!nlh)
> > > +             return -EMSGSIZE;
> > > +
> > > +     ifm = nlmsg_data(nlh);
> > > +     ifm->ifa_family = AF_INET6;
> > > +     ifm->ifa_prefixlen = 128;
> > > +     ifm->ifa_flags = IFA_F_PERMANENT;
> > > +     ifm->ifa_scope = RT_SCOPE_UNIVERSE;
> > > +     ifm->ifa_index = dev->ifindex;
> > > +
> > > +     if (nla_put_in6_addr(skb, IFA_MULTICAST, addr) < 0) {
> > > +             nlmsg_cancel(skb, nlh);
> > > +             return -EMSGSIZE;
> > > +     }
> > > +
> > > +     nlmsg_end(skb, nlh);
> > > +     return 0;
> > > +}
> >
> > Is there a strong reason you reimplement this instead of trying to reuse
> > inet6_fill_ifmcaddr() ? Keeping notifications and get responses in sync
> > used to be a thing in rtnetlink, this code already diverged but maybe
> > we can bring it back.
> >
> > > +static void inet6_ifmcaddr_notify(struct net_device *dev,
> > > +                               const struct in6_addr *addr, int event)
> > > +{
> > > +     struct net *net = dev_net(dev);
> > > +     struct sk_buff *skb;
> > > +     int err = -ENOBUFS;
> >
> > ENOMEM ? I could be wrong but in atomic context the memory pressure
> > can well be transient, it's not like the socket queue filled up.
> >
> > > +     skb = nlmsg_new(NLMSG_ALIGN(sizeof(struct ifaddrmsg))
> > > +                     + nla_total_size(16), GFP_ATOMIC);
> >
> > nit: + goes to the end of previous line
> >
> > > +     if (!skb)
> > > +             goto error;
> > > +
> > > +     err = inet6_fill_ifmcaddr(skb, dev, addr, event);
> > > +     if (err < 0) {
> > > +             WARN_ON_ONCE(err == -EMSGSIZE);
> > > +             kfree_skb(skb);
> >
> > nit: nlmsg_free(), since it exists
> >
> > > +             goto error;
> > > +     }
> > > +
> > > +     rtnl_notify(skb, net, 0, RTNLGRP_IPV6_MCADDR, NULL, GFP_ATOMIC);
> > > +     return;
> > > +error:
> > > +     rtnl_set_sk_err(net, RTNLGRP_IPV6_MCADDR, err);
> > > +}
> > > +
> > --
> > pw-bot: cr