by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH 5/5] connector: Create connector per namespace

Hi Matt,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on ipvs/master]
[also build test WARNING on dm/for-next linux/master linus/master v5.8-rc3 next-20200701]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/Matt-Bennett/RFC-connector-Add-network-namespace-awareness/20200702-083030
base: https://git.kernel.org/pub/scm/linux/kernel/git/horms/ipvs.git master
config: powerpc-pmac32_defconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

drivers/connector/connector.c: In function 'cn_netlink_send_mult':
>> drivers/connector/connector.c:71:18: warning: comparison is always false due to limited range of data type [-Wtype-limits]
71 | if (!msg || len < 0)
| ^
drivers/connector/connector.c: At top level:
drivers/connector/connector.c:238:5: warning: no previous prototype for 'cn_add_callback_one' [-Wmissing-prototypes]
238 | int cn_add_callback_one(struct net *net, struct cb_id *id, const char *name,
| ^~~~~~~~~~~~~~~~~~~

vim +71 drivers/connector/connector.c

30
31 /*
32 * Sends mult (multiple) cn_msg at a time.
33 *
34 * msg->seq and msg->ack are used to determine message genealogy.
35 * When someone sends message it puts there locally unique sequence
36 * and random acknowledge numbers. Sequence number may be copied into
37 * nlmsghdr->nlmsg_seq too.
38 *
39 * Sequence number is incremented with each message to be sent.
40 *
41 * If we expect a reply to our message then the sequence number in
42 * received message MUST be the same as in original message, and
43 * acknowledge number MUST be the same + 1.
44 *
45 * If we receive a message and its sequence number is not equal to the
46 * one we are expecting then it is a new message.
47 *
48 * If we receive a message and its sequence number is the same as one
49 * we are expecting but it's acknowledgement number is not equal to
50 * the acknowledgement number in the original message + 1, then it is
51 * a new message.
52 *
53 * If msg->len != len, then additional cn_msg messages are expected following
54 * the first msg.
55 *
56 * The message is sent to, the portid if given, the group if given, both if
57 * both, or if both are zero then the group is looked up and sent there.
58 */
59 int cn_netlink_send_mult(struct net *net, struct cn_msg *msg, u16 len,
60 u32 portid, u32 __group, gfp_t gfp_mask)
61 {
62 struct cn_callback_entry *__cbq;
63 unsigned int size;
64 struct sk_buff *skb;
65 struct nlmsghdr *nlh;
66 struct cn_msg *data;
67 struct cn_dev *dev = &(net->cdev);
68 u32 group = 0;
69 int found = 0;
70
> 71 if (!msg || len < 0)
72 return -EINVAL;
73
74 if (portid || __group) {
75 group = __group;
76 } else {
77 spin_lock_bh(&dev->cbdev->queue_lock);
78 list_for_each_entry(__cbq, &dev->cbdev->queue_list,
79 callback_entry) {
80 if (cn_cb_equal(&__cbq->id.id, &msg->id)) {
81 found = 1;
82 group = __cbq->group;
83 break;
84 }
85 }
86 spin_unlock_bh(&dev->cbdev->queue_lock);
87
88 if (!found)
89 return -ENODEV;
90 }
91
92 if (!portid && !netlink_has_listeners(dev->nls, group))
93 return -ESRCH;
94
95 size = sizeof(*msg) + len;
96
97 skb = nlmsg_new(size, gfp_mask);
98 if (!skb)
99 return -ENOMEM;
100
101 nlh = nlmsg_put(skb, 0, msg->seq, NLMSG_DONE, size, 0);
102 if (!nlh) {
103 kfree_skb(skb);
104 return -EMSGSIZE;
105 }
106
107 data = nlmsg_data(nlh);
108
109 memcpy(data, msg, size);
110
111 NETLINK_CB(skb).dst_group = group;
112
113 if (group)
114 return netlink_broadcast(dev->nls, skb, portid, group,
115 gfp_mask);
116 return netlink_unicast(dev->nls, skb, portid,
117 !gfpflags_allow_blocking(gfp_mask));
118 }
119 EXPORT_SYMBOL_GPL(cn_netlink_send_mult);
120

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (4.93 kB)
.config.gz (24.39 kB)
Download all attachments

2020-07-02 06:54:09

_______________________________________________
kbuild mailing list -- [email protected]
To unsubscribe send an email to [email protected]

Attachments:

(No filename) (5.87 kB)
.config.gz (26.79 kB)
(No filename) (152.00 B)
Download all attachments

2020-07-02 19:07:13

by Eric W. Biederman

[permalink] [raw]

Subject: Re: [PATCH 0/5] RFC: connector: Add network namespace awareness

Matt Bennett <[email protected]> writes:

> Previously the connector functionality could only be used by processes running in the
> default network namespace. This meant that any process that uses the connector functionality
> could not operate correctly when run inside a container. This is a draft patch series that
> attempts to now allow this functionality outside of the default network namespace.
>
> I see this has been discussed previously [1], but am not sure how my changes relate to all
> of the topics discussed there and/or if there are any unintended side
> effects from my draft

In a quick skim this patchset does not look like it approaches a correct
conversion to having code that works in multiple namespaces.

I will take the changes to proc_id_connector for example.
You report the values in the callers current namespaces.

Which means an unprivileged user can create a user namespace and get
connector to report whichever ids they want to users in another
namespace. AKA lie.

So this appears to make connector completely unreliable.

Eric

> changes.
>
> Thanks.
>
> [1] https://marc.info/?l=linux-kernel&m=150806196728365&w=2
>
> Matt Bennett (5):
> connector: Use task pid helpers
> connector: Use 'current_user_ns' function
> connector: Ensure callback entry is released
> connector: Prepare for supporting multiple namespaces
> connector: Create connector per namespace
>
> Documentation/driver-api/connector.rst | 6 +-
> drivers/connector/cn_proc.c | 110 +++++++-------
> drivers/connector/cn_queue.c | 9 +-
> drivers/connector/connector.c | 192 ++++++++++++++++++++-----
> drivers/hv/hv_fcopy.c | 1 +
> drivers/hv/hv_utils_transport.c | 6 +-
> drivers/md/dm-log-userspace-transfer.c | 6 +-
> drivers/video/fbdev/uvesafb.c | 8 +-
> drivers/w1/w1_netlink.c | 19 +--
> include/linux/connector.h | 38 +++--
> include/net/net_namespace.h | 4 +
> kernel/exit.c | 2 +-
> samples/connector/cn_test.c | 6 +-
> 13 files changed, 286 insertions(+), 121 deletions(-)

2020-07-02 19:11:48

by Christian Brauner

[permalink] [raw]

Subject: Re: [PATCH 0/5] RFC: connector: Add network namespace awareness

On Thu, Jul 02, 2020 at 08:17:38AM -0500, Eric W. Biederman wrote:
> Matt Bennett <[email protected]> writes:
>
> > Previously the connector functionality could only be used by processes running in the
> > default network namespace. This meant that any process that uses the connector functionality
> > could not operate correctly when run inside a container. This is a draft patch series that
> > attempts to now allow this functionality outside of the default network namespace.
> >
> > I see this has been discussed previously [1], but am not sure how my changes relate to all
> > of the topics discussed there and/or if there are any unintended side effects from my draft
> > changes.
>
> Is there a piece of software that uses connector that you want to get
> working in containers?
>
> I am curious what the motivation is because up until now there has been
> nothing very interesting using this functionality. So it hasn't been
> worth anyone's time to make the necessary changes to the code.

Imho, we should just state once and for all that the proc connector will
not be namespaced. This is such a corner-case thing and has been
non-namespaced for such a long time without consistent push for it to be
namespaced combined with the fact that this needs quite some code to
make it work correctly that I fear we end up buying more bugs than we're
selling features. And realistically, you and I will end up maintaining
this and I feel this is not worth the time(?). Maybe I'm being too
pessimistic though.

Christian

2020-07-02 22:45:38

On 2020-07-13, Eric W. Biederman <[email protected]> wrote:
> Matt Bennett <[email protected]> writes:
>
> > On Thu, 2020-07-02 at 21:10 +0200, Christian Brauner wrote:
> >> On Thu, Jul 02, 2020 at 08:17:38AM -0500, Eric W. Biederman wrote:
> >> > Matt Bennett <[email protected]> writes:
> >> >
> >> > > Previously the connector functionality could only be used by processes running in the
> >> > > default network namespace. This meant that any process that uses the connector functionality
> >> > > could not operate correctly when run inside a container. This is a draft patch series that
> >> > > attempts to now allow this functionality outside of the default network namespace.
> >> > >
> >> > > I see this has been discussed previously [1], but am not sure how my changes relate to all
> >> > > of the topics discussed there and/or if there are any unintended side effects from my draft
> >> > > changes.
> >> >
> >> > Is there a piece of software that uses connector that you want to get
> >> > working in containers?
> >
> > We have an IPC system [1] where processes can register their socket
> > details (unix, tcp, tipc, ...) to a 'monitor' process. Processes can
> > then get notified when other processes they are interested in
> > start/stop their servers and use the registered details to connect to
> > them. Everything works unless a process crashes, in which case the
> > monitoring process never removes their details. Therefore the
> > monitoring process uses the connector functionality with
> > PROC_EVENT_EXIT to detect when a process crashes and removes the
> > details if it is a previously registered PID.
> >
> > This was working for us until we tried to run our system in a container.
> >
> >> >
> >> > I am curious what the motivation is because up until now there has been
> >> > nothing very interesting using this functionality. So it hasn't been
> >> > worth anyone's time to make the necessary changes to the code.
> >>
> >> Imho, we should just state once and for all that the proc connector will
> >> not be namespaced. This is such a corner-case thing and has been
> >> non-namespaced for such a long time without consistent push for it to be
> >> namespaced combined with the fact that this needs quite some code to
> >> make it work correctly that I fear we end up buying more bugs than we're
> >> selling features. And realistically, you and I will end up maintaining
> >> this and I feel this is not worth the time(?). Maybe I'm being too
> >> pessimistic though.
> >>
> >
> > Fair enough. I can certainly look for another way to detect process
> > crashes. Interestingly I found a patch set [2] on the mailing list
> > that attempts to solve the problem I wish to solve, but it doesn't
> > look like the patches were ever developed further. From reading the
> > discussion thread on that patch set it appears that I should be doing
> > some form of polling on the /proc files.
>
> Recently Christian Brauner implemented pidfd complete with a poll
> operation that reports when a process terminates.
>
> If you are willing to change your userspace code switching to pidfd
> should be all that you need.

While this does solve the problem of getting exit notifications in
general, you cannot get the exit code. But if they don't care about that
then we can solve that problem another time. :D

--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Attachments:

(No filename) (3.46 kB)
signature.asc (235.00 B)
Download all attachments

2020-07-14 05:19:40

by Matt Bennett

[permalink] [raw]

Subject: Re: [PATCH 0/5] RFC: connector: Add network namespace awareness

On Tue, 2020-07-14 at 15:03 +1000, Aleksa Sarai wrote:
> On 2020-07-13, Eric W. Biederman <[email protected]> wrote:
> > Matt Bennett <[email protected]> writes:
> >
> > > On Thu, 2020-07-02 at 21:10 +0200, Christian Brauner wrote:
> > > > On Thu, Jul 02, 2020 at 08:17:38AM -0500, Eric W. Biederman wrote:
> > > > > Matt Bennett <[email protected]> writes:
> > > > >
> > > > > > Previously the connector functionality could only be used by processes running in the
> > > > > > default network namespace. This meant that any process that uses the connector functionality
> > > > > > could not operate correctly when run inside a container. This is a draft patch series that
> > > > > > attempts to now allow this functionality outside of the default network namespace.
> > > > > >
> > > > > > I see this has been discussed previously [1], but am not sure how my changes relate to all
> > > > > > of the topics discussed there and/or if there are any unintended side effects from my draft
> > > > > > changes.
> > > > >
> > > > > Is there a piece of software that uses connector that you want to get
> > > > > working in containers?
> > >
> > > We have an IPC system [1] where processes can register their socket
> > > details (unix, tcp, tipc, ...) to a 'monitor' process. Processes can
> > > then get notified when other processes they are interested in
> > > start/stop their servers and use the registered details to connect to
> > > them. Everything works unless a process crashes, in which case the
> > > monitoring process never removes their details. Therefore the
> > > monitoring process uses the connector functionality with
> > > PROC_EVENT_EXIT to detect when a process crashes and removes the
> > > details if it is a previously registered PID.
> > >
> > > This was working for us until we tried to run our system in a container.
> > >
> > > > >
> > > > > I am curious what the motivation is because up until now there has been
> > > > > nothing very interesting using this functionality. So it hasn't been
> > > > > worth anyone's time to make the necessary changes to the code.
> > > >
> > > > Imho, we should just state once and for all that the proc connector will
> > > > not be namespaced. This is such a corner-case thing and has been
> > > > non-namespaced for such a long time without consistent push for it to be
> > > > namespaced combined with the fact that this needs quite some code to
> > > > make it work correctly that I fear we end up buying more bugs than we're
> > > > selling features. And realistically, you and I will end up maintaining
> > > > this and I feel this is not worth the time(?). Maybe I'm being too
> > > > pessimistic though.
> > > >
> > >
> > > Fair enough. I can certainly look for another way to detect process
> > > crashes. Interestingly I found a patch set [2] on the mailing list
> > > that attempts to solve the problem I wish to solve, but it doesn't
> > > look like the patches were ever developed further. From reading the
> > > discussion thread on that patch set it appears that I should be doing
> > > some form of polling on the /proc files.
> >
> > Recently Christian Brauner implemented pidfd complete with a poll
> > operation that reports when a process terminates.
> >
> > If you are willing to change your userspace code switching to pidfd
> > should be all that you need.
>
> While this does solve the problem of getting exit notifications in
> general, you cannot get the exit code. But if they don't care about that
> then we can solve that problem another time. :D
>

From first glance using pidfd will do exactly what we need. Not being able
to get the exit code will not be an issue. In fact I think it will be an
improvement over the connector as the listener will now only be waiting for
the PIDs we actually care about - rather than getting woken up on every single
process exit and having to check if it cares about the PID.

Many thanks Eric and others,
Matt