| Message ID | 20231206213332.55565-7-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs-utils: handle BAD_INTEGRITY ERROR | expand |
On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: > From: Olga Kornievskaia <kolga@netapp.com> > > If we have rpc_gss_sccreate in tirpc library define > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity > errors. > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > --- > aclocal/libtirpc.m4 | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 > index bddae022..ef48a2ae 100644 > --- a/aclocal/libtirpc.m4 > +++ b/aclocal/libtirpc.m4 > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ > [Define to 1 if your tirpc library provides libtirpc_set_debug])],, > [${LIBS}])]) > > + AS_IF([test -n "${LIBTIRPC}"], > + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], > + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], > + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, > + [${LIBS}])]) > AC_SUBST([AM_CPPFLAGS]) > AC_SUBST(LIBTIRPC) It would be better for distributors if this checked that the local version of libtirpc has the rpc_gss_seccreate fix that you sent. The PKG_CHECK_MODULES macro should work for that, once you know the version number of libtirpc that will have that fix. Also, this patch should come either before "gssd: switch to using rpc_gss_seccreate()" or this change should be squashed into that patch, IMO.
On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: > > From: Olga Kornievskaia <kolga@netapp.com> > > > > If we have rpc_gss_sccreate in tirpc library define > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity > > errors. > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > --- > > aclocal/libtirpc.m4 | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 > > index bddae022..ef48a2ae 100644 > > --- a/aclocal/libtirpc.m4 > > +++ b/aclocal/libtirpc.m4 > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ > > [Define to 1 if your tirpc library provides libtirpc_set_debug])],, > > [${LIBS}])]) > > > > + AS_IF([test -n "${LIBTIRPC}"], > > + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], > > + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], > > + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, > > + [${LIBS}])]) > > AC_SUBST([AM_CPPFLAGS]) > > AC_SUBST(LIBTIRPC) > > It would be better for distributors if this checked that the local > version of libtirpc has the rpc_gss_seccreate fix that you sent. > The PKG_CHECK_MODULES macro should work for that, once you know the > version number of libtirpc that will have that fix. > > Also, this patch should come either before "gssd: switch to using > rpc_gss_seccreate()" or this change should be squashed into that > patch, IMO. I can certainly re-arrange the order (if Steve wants me to re-send an ordered list). I attempted to address your comment to check for existence of the function or fallback to the old way. I'm not sure I'm capable of producing something that depends on distro versioning (or am I supposed to be)? I think this goes back to me hoping that a distro would create matching set of libtirpc and nfs-utils rpms... Thank you for the reviews! > > > -- > Chuck Lever
On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote: > On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > > > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > If we have rpc_gss_sccreate in tirpc library define > > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity > > > errors. > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > --- > > > aclocal/libtirpc.m4 | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 > > > index bddae022..ef48a2ae 100644 > > > --- a/aclocal/libtirpc.m4 > > > +++ b/aclocal/libtirpc.m4 > > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ > > > [Define to 1 if your tirpc library provides libtirpc_set_debug])],, > > > [${LIBS}])]) > > > > > > + AS_IF([test -n "${LIBTIRPC}"], > > > + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], > > > + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], > > > + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, > > > + [${LIBS}])]) > > > AC_SUBST([AM_CPPFLAGS]) > > > AC_SUBST(LIBTIRPC) > > > > It would be better for distributors if this checked that the local > > version of libtirpc has the rpc_gss_seccreate fix that you sent. > > The PKG_CHECK_MODULES macro should work for that, once you know the > > version number of libtirpc that will have that fix. > > > > Also, this patch should come either before "gssd: switch to using > > rpc_gss_seccreate()" or this change should be squashed into that > > patch, IMO. > > I can certainly re-arrange the order (if Steve wants me to re-send an > ordered list). I attempted to address your comment to check for > existence of the function or fallback to the old way. A comment that I made when I thought no changes to rpc_gss_seccreate(3t) would be needed.... But you found and fixed a bug there. > I'm not sure I'm > capable of producing something that depends on distro versioning (or > am I supposed to be)? I think this series truly needs to check the libtirpc version. Otherwise the build will complete successfully, gssd will use rpc_gss_seccreate(), but it will be broken. Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you should find a pattern to use. > I think this goes back to me hoping that a > distro would create matching set of libtirpc and nfs-utils rpms... IME distros don't work that way.
On Thu, Dec 7, 2023 at 5:27 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote: > > On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > > > > > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > If we have rpc_gss_sccreate in tirpc library define > > > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity > > > > errors. > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > > --- > > > > aclocal/libtirpc.m4 | 5 +++++ > > > > 1 file changed, 5 insertions(+) > > > > > > > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 > > > > index bddae022..ef48a2ae 100644 > > > > --- a/aclocal/libtirpc.m4 > > > > +++ b/aclocal/libtirpc.m4 > > > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ > > > > [Define to 1 if your tirpc library provides libtirpc_set_debug])],, > > > > [${LIBS}])]) > > > > > > > > + AS_IF([test -n "${LIBTIRPC}"], > > > > + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], > > > > + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], > > > > + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, > > > > + [${LIBS}])]) > > > > AC_SUBST([AM_CPPFLAGS]) > > > > AC_SUBST(LIBTIRPC) > > > > > > It would be better for distributors if this checked that the local > > > version of libtirpc has the rpc_gss_seccreate fix that you sent. > > > The PKG_CHECK_MODULES macro should work for that, once you know the > > > version number of libtirpc that will have that fix. > > > > > > Also, this patch should come either before "gssd: switch to using > > > rpc_gss_seccreate()" or this change should be squashed into that > > > patch, IMO. > > > > I can certainly re-arrange the order (if Steve wants me to re-send an > > ordered list). I attempted to address your comment to check for > > existence of the function or fallback to the old way. > > A comment that I made when I thought no changes to rpc_gss_seccreate(3t) > would be needed.... But you found and fixed a bug there. > > > > I'm not sure I'm > > capable of producing something that depends on distro versioning (or > > am I supposed to be)? > > I think this series truly needs to check the libtirpc version. > Otherwise the build will complete successfully, gssd will use > rpc_gss_seccreate(), but it will be broken. > > Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you > should find a pattern to use. Yes but I won't know the version number of libtirpc (version or rpm package) for which to check? It seems like libtirpc changes needs to be checked in (btw I'm assuming a new version would need to be generated), then (if that's it or libtirpc version and package version are different things there might be more) this particular patch could be generated. Isn't that correct? Steve, I could really use your guidance on steps to be done here. Thank you. > > > > I think this goes back to me hoping that a > > distro would create matching set of libtirpc and nfs-utils rpms... > > IME distros don't work that way. > > > -- > Chuck Lever
On 12/7/23 9:44 AM, Chuck Lever wrote: > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: >> From: Olga Kornievskaia <kolga@netapp.com> >> >> If we have rpc_gss_sccreate in tirpc library define >> HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity >> errors. >> >> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >> --- >> aclocal/libtirpc.m4 | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 >> index bddae022..ef48a2ae 100644 >> --- a/aclocal/libtirpc.m4 >> +++ b/aclocal/libtirpc.m4 >> @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ >> [Define to 1 if your tirpc library provides libtirpc_set_debug])],, >> [${LIBS}])]) >> >> + AS_IF([test -n "${LIBTIRPC}"], >> + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], >> + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], >> + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, >> + [${LIBS}])]) >> AC_SUBST([AM_CPPFLAGS]) >> AC_SUBST(LIBTIRPC) > > It would be better for distributors if this checked that the local > version of libtirpc has the rpc_gss_seccreate fix that you sent. > The PKG_CHECK_MODULES macro should work for that, once you know the > version number of libtirpc that will have that fix. This is some the distro need to worried about... Not upstream. Yes... do to the recent changes to libtirpc, I did need to add a requirement to nfs-utils (in Fedora) so it would compile. > > Also, this patch should come either before "gssd: switch to using > rpc_gss_seccreate()" or this change should be squashed into that > patch, IMO.Taking a quick look... The patches seem fine... but I will take a closer look over the weekend. steved >
On 12/8/23 9:26 AM, Olga Kornievskaia wrote: > On Thu, Dec 7, 2023 at 5:27 PM Chuck Lever <chuck.lever@oracle.com> wrote: >> >> On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote: >>> On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: >>>> >>>> On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: >>>>> From: Olga Kornievskaia <kolga@netapp.com> >>>>> >>>>> If we have rpc_gss_sccreate in tirpc library define >>>>> HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity >>>>> errors. >>>>> >>>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com> >>>>> --- >>>>> aclocal/libtirpc.m4 | 5 +++++ >>>>> 1 file changed, 5 insertions(+) >>>>> >>>>> diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 >>>>> index bddae022..ef48a2ae 100644 >>>>> --- a/aclocal/libtirpc.m4 >>>>> +++ b/aclocal/libtirpc.m4 >>>>> @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ >>>>> [Define to 1 if your tirpc library provides libtirpc_set_debug])],, >>>>> [${LIBS}])]) >>>>> >>>>> + AS_IF([test -n "${LIBTIRPC}"], >>>>> + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], >>>>> + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], >>>>> + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, >>>>> + [${LIBS}])]) >>>>> AC_SUBST([AM_CPPFLAGS]) >>>>> AC_SUBST(LIBTIRPC) >>>> >>>> It would be better for distributors if this checked that the local >>>> version of libtirpc has the rpc_gss_seccreate fix that you sent. >>>> The PKG_CHECK_MODULES macro should work for that, once you know the >>>> version number of libtirpc that will have that fix. >>>> >>>> Also, this patch should come either before "gssd: switch to using >>>> rpc_gss_seccreate()" or this change should be squashed into that >>>> patch, IMO. >>> >>> I can certainly re-arrange the order (if Steve wants me to re-send an >>> ordered list). I attempted to address your comment to check for >>> existence of the function or fallback to the old way. >> >> A comment that I made when I thought no changes to rpc_gss_seccreate(3t) >> would be needed.... But you found and fixed a bug there. >> >> >>> I'm not sure I'm >>> capable of producing something that depends on distro versioning (or >>> am I supposed to be)? >> >> I think this series truly needs to check the libtirpc version. >> Otherwise the build will complete successfully, gssd will use >> rpc_gss_seccreate(), but it will be broken. >> >> Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you >> should find a pattern to use. > > Yes but I won't know the version number of libtirpc (version or rpm > package) for which to check? It seems like libtirpc changes needs to > be checked in (btw I'm assuming a new version would need to be > generated), then (if that's it or libtirpc version and package version > are different things there might be more) this particular patch could > be generated. Isn't that correct? > > Steve, I could really use your guidance on steps to be done here. Again... The versions "on who is on first and what is on second" :-) Is not an upstream problem... It is a distro problem... Let me take a closer look... > > Thank you. > >> >> >>> I think this goes back to me hoping that a >>> distro would create matching set of libtirpc and nfs-utils rpms... We do... upstream creates tar balls... distros create rpm that have requirements for certain versions of things. steved. >> >> IME distros don't work that way. >> >> >> -- >> Chuck Lever >
On Fri, Dec 08, 2023 at 10:01:29AM -0500, Steve Dickson wrote: > > > On 12/8/23 9:26 AM, Olga Kornievskaia wrote: > > On Thu, Dec 7, 2023 at 5:27 PM Chuck Lever <chuck.lever@oracle.com> wrote: > > > > > > On Thu, Dec 07, 2023 at 05:21:50PM -0500, Olga Kornievskaia wrote: > > > > On Thu, Dec 7, 2023 at 9:44 AM Chuck Lever <chuck.lever@oracle.com> wrote: > > > > > > > > > > On Wed, Dec 06, 2023 at 04:33:32PM -0500, Olga Kornievskaia wrote: > > > > > > From: Olga Kornievskaia <kolga@netapp.com> > > > > > > > > > > > > If we have rpc_gss_sccreate in tirpc library define > > > > > > HAVE_TIRPC_GSS_SECCREATE, which would allow us to handle bad_integrity > > > > > > errors. > > > > > > > > > > > > Signed-off-by: Olga Kornievskaia <kolga@netapp.com> > > > > > > --- > > > > > > aclocal/libtirpc.m4 | 5 +++++ > > > > > > 1 file changed, 5 insertions(+) > > > > > > > > > > > > diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 > > > > > > index bddae022..ef48a2ae 100644 > > > > > > --- a/aclocal/libtirpc.m4 > > > > > > +++ b/aclocal/libtirpc.m4 > > > > > > @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ > > > > > > [Define to 1 if your tirpc library provides libtirpc_set_debug])],, > > > > > > [${LIBS}])]) > > > > > > > > > > > > + AS_IF([test -n "${LIBTIRPC}"], > > > > > > + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], > > > > > > + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], > > > > > > + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, > > > > > > + [${LIBS}])]) > > > > > > AC_SUBST([AM_CPPFLAGS]) > > > > > > AC_SUBST(LIBTIRPC) > > > > > > > > > > It would be better for distributors if this checked that the local > > > > > version of libtirpc has the rpc_gss_seccreate fix that you sent. > > > > > The PKG_CHECK_MODULES macro should work for that, once you know the > > > > > version number of libtirpc that will have that fix. > > > > > > > > > > Also, this patch should come either before "gssd: switch to using > > > > > rpc_gss_seccreate()" or this change should be squashed into that > > > > > patch, IMO. > > > > > > > > I can certainly re-arrange the order (if Steve wants me to re-send an > > > > ordered list). I attempted to address your comment to check for > > > > existence of the function or fallback to the old way. > > > > > > A comment that I made when I thought no changes to rpc_gss_seccreate(3t) > > > would be needed.... But you found and fixed a bug there. > > > > > > > > > > I'm not sure I'm > > > > capable of producing something that depends on distro versioning (or > > > > am I supposed to be)? > > > > > > I think this series truly needs to check the libtirpc version. > > > Otherwise the build will complete successfully, gssd will use > > > rpc_gss_seccreate(), but it will be broken. > > > > > > Grep for PKG_CHECK_MODULES in the other files in aclocal/ and you > > > should find a pattern to use. > > > > Yes but I won't know the version number of libtirpc (version or rpm > > package) for which to check? It seems like libtirpc changes needs to > > be checked in (btw I'm assuming a new version would need to be > > generated), then (if that's it or libtirpc version and package version > > are different things there might be more) this particular patch could > > be generated. Isn't that correct? 1. Commit the reverts to nfs-utils, and cut a release. This enables nfs-utils to build everywhere again -- it addresses an immediate bug. 2. Commit your libtirpc patches, and cut a release. This fixes the ABI issue in libtirpc, and you now have a known-good library release version number to use. 3. Update the libtirpc aclocal check in nfs-utils to use that version number, and commit the rest of your fixes. This fix will then appear in the next nfs-utils release. > > Steve, I could really use your guidance on steps to be done here. > Again... The versions "on who is on first and what is on second" :-) > Is not an upstream problem... It is a distro problem... I think distros will be less likely to upgrade if there are LEGO blocks laying on the floor that they accidentally step on. And my impression is that distros want the config to break rather than that hidden bugs leak into their production builds. This is clearly something upstream can flag, and we should because otherwise, the breakage can be silent or frustrating to debug. This is a security issue, really, since it directly involves gssd. I mean, why bother to have all of the autoconf machinery if upstream doesn't care about checking library versions? > Let me take a closer look... > > > > > Thank you. > > > > > > > > > > > > I think this goes back to me hoping that a > > > > distro would create matching set of libtirpc and nfs-utils rpms... > We do... upstream creates tar balls... distros create rpm > that have requirements for certain versions of things. That is typically the case. I'm concerned about the few times it isn't, or if there are testing gaps.
diff --git a/aclocal/libtirpc.m4 b/aclocal/libtirpc.m4 index bddae022..ef48a2ae 100644 --- a/aclocal/libtirpc.m4 +++ b/aclocal/libtirpc.m4 @@ -26,6 +26,11 @@ AC_DEFUN([AC_LIBTIRPC], [ [Define to 1 if your tirpc library provides libtirpc_set_debug])],, [${LIBS}])]) + AS_IF([test -n "${LIBTIRPC}"], + [AC_CHECK_LIB([tirpc], [rpc_gss_seccreate], + [AC_DEFINE([HAVE_TIRPC_GSS_SECCREATE], [1], + [Define to 1 if your tirpc library provides rpc_gss_seccreate])],, + [${LIBS}])]) AC_SUBST([AM_CPPFLAGS]) AC_SUBST(LIBTIRPC)