Re: hyperv: is register a mandatory phase by hypervkvpd?

[vaughan <vaughan.cao@xxxxxxxxxx>]

于 2013年03月06日 22:23, KY Srinivasan 写道:
> > -----Original Message-----
> > From: vaughan [mailto:vaughan.cao@xxxxxxxxxx]
> > Sent: Tuesday, March 05, 2013 9:09 PM
> > To: KY Srinivasan
> > Cc: Xitao Cao;devel@xxxxxxxxxxxxxxxxxxxxxx; Haiyang Zhang
> > Subject: Re: hyperv: is register a mandatory phase by hypervkvpd?
> >
> > 于 2013年03月06日 01:02, Xitao Cao 写道:
> > > On Tue, Mar 5, 2013 at 11:42 PM, KY Srinivasan<kys@xxxxxxxxxxxxx>  wrote:
> > > > > -----Original Message-----
> > > > > From: vaughan [mailto:vaughan.cao@xxxxxxxxxx]
> > > > > Sent: Tuesday, March 05, 2013 7:48 AM
> > > > > To: KY Srinivasan
> > > > > Cc:devel@xxxxxxxxxxxxxxxxxxxxxx; Haiyang Zhang;xitao.cao@xxxxxxxxx
> > > > > Subject: hyperv: is register a mandatory phase by hypervkvpd?
> > > > >
> > > > > I guess I found a bug -- hypervkvpd running alone without hv_utils
> > > > > loaded encounters segfault when service cgred start on RHEL6.4. It
> > > > > occurs with both 0.8 and 0.9, regardless of i686 or x86_64.
> > > > >
> > > > > I read in hv_kvp_daemon.c that the user mode componet should first
> > > > > registers with the kernel component.
> > > > > But in my test, the hand shake phase has been ignored.
> > > > > Things happens like this:
> > > > > hv_utils.ko and hv_vmbus.ko is not loaded, start hypervkvpd is fine.
> > > > > Then, I start cgred with the default configuration. cgroup also use
> > > > > NETLINK_CONNECTOR protocol and send messages with cb_id{1,1}.
> > Hypervkvpd
> > > > > receive messages without checking their source. Some messages with
> > > > > cb_id{1,1} were receviced and blindly interpreted as hv_kvp_msg. Since
> > > > > the hand_shake check is as below:
> Jason,
> There appears to be a connector related issue here. Could you
> check to see if the connector code is current.
I suppose it's not about connector. It's because you subscribed both 
channel 8 and 1.(9=8+1)
I found the following posts:
http://linux.derkeiler.com/Mailing-Lists/Kernel/2006-04/msg03068.html
http://linux.derkeiler.com/Mailing-Lists/Kernel/2006-04/msg03876.html

bind() nladdr value is a bitmask of groups, not a single group number,
it was done for backward compatibility, so bind(5) is equal to
subscribe(1) and subscribe(3). That is why you saw messages without
subscription


    l_local.nl_family = AF_NETLINK;
    l_local.nl_groups = 0x3;
    l_local.nl_pid = getpid();

    if (bind(s, (struct sockaddr *)&l_local, sizeof(struct 
sockaddr_nl)) == -1) {
    perror("bind");
    close(s);
    return -1;
    }
    int on = l_local.nl_groups;
    if (setsockopt(s, 270, 1, &on, sizeof(on))) {
    perror("setsockopt");
    close(s);
    return -1;
    }


In this example you will receive messages for groups 1, 2 (bind time
gropus 0x3 is equal to (1<<(1-1)) | (1<<(2-1))) and group 3 (you
have subscribed to that gropu explicitly).


unsigned int netlink_group_mask(unsigned int group)
{
    return group ? 1 << (group - 1) : 0;
}


Regards,
Vaughan

> Regards,
>
> K. Y
>
> There is some connector related issue here. Jason, could you check
> > > > > if ((in_hand_shake) && (op == KVP_OP_REGISTER1)) {
> > > > > ...
> > > > > continue;
> > > > > }
> > > > > //handle kvp messages
> > > > > switch (op) { ... }
> > > > > Register phase is also skipped.
> > > > > Everytime the KVP_OP_SET opcode is reached, kvp_key_add_or_modify() is
> > > > > invoked with an very large key_size. After several iterations, segfault
> > > > > occurs in memcpy(record[i].key, key, key_size) (key_size is negative now).
> > > > >
> > > > > I'm not very familiar with connector. But I ran the sample in
> > > > > Documentation/connector/ and found that a NETLINK_CONNECTOR socket
> > > > > would
> > > > > always some messages with cb_id{1,1}. So blindly suppose all messages
> > > > > are kvp_msg is not correct. hypervkvpd should check the source of
> > > > > messages and perhaps even check nlmsg_type in the nlmsghdr.
> > > > The current code does use recvfrom() and checks the sending PID to see if it is
> > trusted.
> > > > What version of the code are you testing with.
> > > Tested both 0.8.0.1 and 0.9.0.1 (redhat version) on 2.6.32-343 on RHEL6.4.
> > > I confirm PID is zero, but I think it only means it is sent by kernel.
> > > I have no idea whether it is correct for a NETLINK_CONNECTOR
> > > implementation that a socket with id {9,1} is able to receive messages
> > > with id {1,1}.
> > I'm sorry it should be 0-0.9 and 0-0.8 RHEL version. .spec is as below:
> >
> > # HyperV KVP daemon name
> > %global hv_kvp_daemon hv_kvp_daemon
> >
> > Name:     hypervkvpd
> > Version:  0
> > Release:  0.9%{?dist}
> > Summary:  HyperV key value pair (KVP) daemon
> >
> > Group:    System Environment/Daemons
> > License:  GPLv2
> > URL:http://www.kernel.org
> > # Source file obtained from kernel upstream.
> > # git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > # The daemon and scripts are located in "master branch - /tools/hv"
> > Source0:  %{hv_kvp_daemon}.c
> > Source1:  hv_get_dhcp_info.sh
> > Source2:  hv_get_dns_info.sh
> > Source3:  hv_set_ifconfig.sh
> > Source4:  hypervkvpd
> >
> > # Correct paths to external scripts ("/usr/libexec/hypervkvpd").
> > Patch0:   hypervkvpd-0-corrected_paths_to_external_scripts.patch
> > # rhbz#872593
> > Patch1: hypervkvpd-0-Netlink-source-address-validation-allows-DoS.patch
> > # rhbz#872566
> > Patch2:   hypervkvpd-0-long_file_names_from_readdir.patch
> > # rhbz#872584
> > Patch3:   hypervkvpd-0-var_subdirectory.patch
> > Patch4:   hypervkvpd-0-string_types.patch
> > Patch5: hypervkvpd-0-permissions_of_created_directory_and_files.patch
> > # rhbz#890301
> > Patch6:   hypervkvpd-0-fix_a_typo_in_hv_set_ifconfig_sh.patch
> > Patch7:   hypervkvpd-0-Fix-how-ifcfg-file-is-created.patch
> > Patch8:   hypervkvpd-0-Use-CLOEXEC-when-opening-kvp_pool-files.patch
> >
> > BuildRoot: %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
> > # this package is supposed to run on RHEL when it's a guest under Hyper-V
> > ExclusiveArch:    x86_64 i686
> > BuildRequires:    kernel-devel >= 2.6.32-336
> > Requires(post):   chkconfig
> > Requires(preun):  chkconfig
> > # This is for /sbin/service
> > Requires(preun):  initscripts
> > Requires(postun): initscripts
> >
> > %description
> > Hypervkvpd is an implementation of HyperV key value pair (KVP)
> > functionality for Linux.
> >
> >
> > %prep
> > %setup -Tc
> > cp -pvL %{SOURCE0} %{hv_kvp_daemon}.c
> > cp -pvL %{SOURCE1} hv_get_dhcp_info.sh
> > cp -pvL %{SOURCE2} hv_get_dns_info.sh
> > cp -pvL %{SOURCE3} hv_set_ifconfig.sh
> > cp -pvL %{SOURCE4} hypervkvpd
> >
> > %patch0 -p1 -b .external_scripts
> > %patch1 -p1 -b .netlink_DoS
> > %patch2 -p1 -b .long_names
> > %patch3 -p1 -b .var_subdir
> > %patch4 -p1 -b .string_types
> > %patch5 -p1 -b .permissions
> > %patch6 -p1
> > %patch7 -p3 -b .ifcfg_fix
> > %patch8 -p3 -b .use_CLOEXEC
> >
> >
> > %build
> > # kernel-devel version
> > %{!?kversion: %global kversion `ls %{_usrsrc}/kernels | sort -dr | head
> > -n 1`}
> >
> > gcc \
> >       %{optflags} \
> >       -I%{_usrsrc}/kernels/%{kversion}/include \
> >       %{hv_kvp_daemon}.c \
> >       -o %{hv_kvp_daemon}
> >
> >
> > %install
> > rm -rf %{buildroot}
> >
> > mkdir -p %{buildroot}%{_sbindir}
> > install -p -m 0755 %{hv_kvp_daemon} %{buildroot}%{_sbindir}
> > mkdir -p %{buildroot}%{_initrddir}
> > # SysV init script
> > install -p -m 0755 %{SOURCE4} %{buildroot}%{_initrddir}
> > # Shell scripts for the daemon
> > mkdir -p %{buildroot}%{_libexecdir}/%{name}
> > install -p -m 0755 hv_get_dhcp_info.sh
> > %{buildroot}%{_libexecdir}/%{name}/hv_get_dhcp_info
> > install -p -m 0755 hv_get_dns_info.sh
> > %{buildroot}%{_libexecdir}/%{name}/hv_get_dns_info
> > install -p -m 0755 hv_set_ifconfig.sh
> > %{buildroot}%{_libexecdir}/%{name}/hv_set_ifconfig
> > # Directory for pool files
> > mkdir -p %{buildroot}%{_sharedstatedir}/hyperv
> >
> >
> > %post
> > # This adds the proper /etc/rc*.d links for the script
> > /sbin/chkconfig --add hypervkvpd
> >
> >
> > %preun
> > # Removing package
> > if [ $1 -eq 0 ] ; then
> >       /sbin/service hypervkvpd stop >/dev/null 2>&1
> >       /sbin/chkconfig --del hypervkvpd
> > fi
> >
> >
> > %postun
> > # Updating package
> > if [ "$1" -ge "1" ] ; then
> >       /sbin/service hypervkvpd condrestart >/dev/null 2>&1 || :
> > fi
> > # If removing the package, delete %%{_sharedstatedir}/hyperv directory
> > if [ "$1" -eq "0" ] ; then
> >       rm -rf %{_sharedstatedir}/hyperv || :
> > fi
> >
> >
> > %clean
> > rm -rf %{buildroot}
> >
> >
> > %files
> > %defattr(-,root,root,-)
> > %doc
> > %{_sbindir}/%{hv_kvp_daemon}
> > %{_initrddir}/hypervkvpd
> > %dir %{_libexecdir}/%{name}
> > %{_libexecdir}/%{name}/hv_get_dhcp_info
> > %{_libexecdir}/%{name}/hv_get_dns_info
> > %{_libexecdir}/%{name}/hv_set_ifconfig
> > %dir %{_sharedstatedir}/hyperv
> >
> >
> > %changelog
> > * Mon Jan 14 2013 Tomas Hozza<thozza@xxxxxxxxxx>  - 0-0.9
> > - Fix a typo in hv_set_ifconfig.sh script
> > - Fix creation process of ifcfg-* files when doing IP injection
> > - Use CLOEXEC when opening pool files to prevent FD leaking
> > - Changes in SPEC file:
> >    - remove/create %%{_sharedstatedir}/hyperv if removing/installing package
> >    - install scripts without ".sh" extension
> >
> > * Mon Nov 12 2012 Tomas Hozza<thozza@xxxxxxxxxx>  - 0-0.8
> > - Bumping release to 0.8 to avoid update path from RHEL5
> > - changing the way how Netlink source address is validated (#872593)
> > - fix for long names from readdir (#872566)
> > - applied reasonable Debian/Ubuntu patches (#872584)
> >
> > * Thu Nov 08 2012 Tomas Hozza<thozza@xxxxxxxxxx>  - 0-0.2
> > - Changing ExclusiveArch just to x86_64 i686 (#870288)
> >
> > * Tue Sep 25 2012 Tomas Hozza<thozza@xxxxxxxxxx>  - 0-0.1
> > - Initial spec file
> >
> >
> > > > Regards,
> > > >
> > > > K. Y
> > > > > --
> > > > > Regards,
> > > > > Vaughan

_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel