AFAICS this is the known problem with weird mips definitions of
__kernel_fsid_t which uses long whereas all other architectures use int,
right? Seeing that mips can actually have 8-byte longs, I guess this
bogosity is just wired in the kernel API and we cannot easily fix it in
mips (mips guys, correct me if I'm wrong). So what if we just
unconditionally typed printed values to unsigned int to silence the
warning?
Honza
On Thu 14-03-19 02:31:52, kbuild test robot wrote:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 5453a3df2a5eb49bc24615d4cf0d66b2aae05e5f
> commit: e9e0c8903009477b630e37a8b6364b26a00720da fanotify: encode file identifier for FAN_REPORT_FID
> date: 5 weeks ago
> config: mips-allmodconfig (attached as .config)
> compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> git checkout e9e0c8903009477b630e37a8b6364b26a00720da
> # save the attached .config to linux build tree
> GCC_VERSION=7.2.0 make.cross ARCH=mips
>
> All warnings (new ones prefixed by >>):
>
> In file included from include/linux/kernel.h:14:0,
> from include/linux/list.h:9,
> from include/linux/preempt.h:11,
> from include/linux/spinlock.h:51,
> from include/linux/fdtable.h:11,
> from fs/notify/fanotify/fanotify.c:3:
> fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
> include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]
> #define KERN_SOH "\001" /* ASCII Start Of Header */
> ^
> include/linux/printk.h:424:10: note: in definition of macro 'printk_ratelimited'
> printk(fmt, ##__VA_ARGS__); \
> ^~~
> include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
> #define KERN_WARNING KERN_SOH "4" /* warning conditions */
> ^~~~~~~~
> include/linux/printk.h:440:21: note: in expansion of macro 'KERN_WARNING'
> printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> ^~~~~~~~~~~~
> >> fs/notify/fanotify/fanotify.c:198:2: note: in expansion of macro 'pr_warn_ratelimited'
> pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> ^~~~~~~~~~~~~~~~~~~
> fs/notify/fanotify/fanotify.c:198:61: note: format string is defined here
> pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> ~^
> %lx
> In file included from include/linux/kernel.h:14:0,
> from include/linux/list.h:9,
> from include/linux/preempt.h:11,
> from include/linux/spinlock.h:51,
> from include/linux/fdtable.h:11,
> from fs/notify/fanotify/fanotify.c:3:
> include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'long int' [-Wformat=]
> #define KERN_SOH "\001" /* ASCII Start Of Header */
> ^
> include/linux/printk.h:424:10: note: in definition of macro 'printk_ratelimited'
> printk(fmt, ##__VA_ARGS__); \
> ^~~
> include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
> #define KERN_WARNING KERN_SOH "4" /* warning conditions */
> ^~~~~~~~
> include/linux/printk.h:440:21: note: in expansion of macro 'KERN_WARNING'
> printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> ^~~~~~~~~~~~
> >> fs/notify/fanotify/fanotify.c:198:2: note: in expansion of macro 'pr_warn_ratelimited'
> pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> ^~~~~~~~~~~~~~~~~~~
> fs/notify/fanotify/fanotify.c:198:64: note: format string is defined here
> pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> ~^
> %lx
>
> vim +/pr_warn_ratelimited +198 fs/notify/fanotify/fanotify.c
>
> 154
> 155 static int fanotify_encode_fid(struct fanotify_event *event,
> 156 const struct path *path, gfp_t gfp)
> 157 {
> 158 struct fanotify_fid *fid = &event->fid;
> 159 int dwords, bytes = 0;
> 160 struct kstatfs stat;
> 161 int err, type;
> 162
> 163 stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
> 164 fid->ext_fh = NULL;
> 165 dwords = 0;
> 166 err = -ENOENT;
> 167 type = exportfs_encode_inode_fh(d_inode(path->dentry), NULL, &dwords,
> 168 NULL);
> 169 if (!dwords)
> 170 goto out_err;
> 171
> 172 err = vfs_statfs(path, &stat);
> 173 if (err)
> 174 goto out_err;
> 175
> 176 bytes = dwords << 2;
> 177 if (bytes > FANOTIFY_INLINE_FH_LEN) {
> 178 /* Treat failure to allocate fh as failure to allocate event */
> 179 err = -ENOMEM;
> 180 fid->ext_fh = kmalloc(bytes, gfp);
> 181 if (!fid->ext_fh)
> 182 goto out_err;
> 183 }
> 184
> 185 type = exportfs_encode_inode_fh(d_inode(path->dentry),
> 186 fanotify_fid_fh(fid, bytes), &dwords,
> 187 NULL);
> 188 err = -EINVAL;
> 189 if (!type || type == FILEID_INVALID || bytes != dwords << 2)
> 190 goto out_err;
> 191
> 192 fid->fsid = stat.f_fsid;
> 193 event->fh_len = bytes;
> 194
> 195 return type;
> 196
> 197 out_err:
> > 198 pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> 199 "type=%d, bytes=%d, err=%i)\n",
> 200 stat.f_fsid.val[0], stat.f_fsid.val[1],
> 201 type, bytes, err);
> 202 kfree(fid->ext_fh);
> 203 fid->ext_fh = NULL;
> 204 event->fh_len = 0;
> 205
> 206 return FILEID_INVALID;
> 207 }
> 208
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Mar 14, 2019 at 10:37 AM Jan Kara <[email protected]> wrote:
>
> AFAICS this is the known problem with weird mips definitions of
> __kernel_fsid_t which uses long whereas all other architectures use int,
> right? Seeing that mips can actually have 8-byte longs, I guess this
> bogosity is just wired in the kernel API and we cannot easily fix it in
> mips (mips guys, correct me if I'm wrong). So what if we just
> unconditionally typed printed values to unsigned int to silence the
> warning?
I don't understand why. To me that sounds like papering over a bug.
See this reply from mips developer Paul Burton:
https://marc.info/?l=linux-fsdevel&m=154783680019904&w=2
mips developers have not replied to the question why __kernel_fsid_t
should use long.
My concern is that we expose __kernel_fsid_t type in uapi header
in struct fanotify_event_info_fid. We should make sure this type
is consistent with glibc's fsid_t.
For reference, the statfs(2) man page says that on Linux
"...fsid_t is defined as struct { int val[2]; }".
Besides, it looks like __kernel_fsid_t got let behind on the
mips posix_types cleanup:
bb8ac181a5cf bury __kernel_nlink_t, make internal nlink_t consistent
86fcd10e9a57 mips: Use generic posix_types.h
To me this seems like an issue that mips developers should
advise on the solution.
Thanks,
Amir.
>
> Honza
>
> On Thu 14-03-19 02:31:52, kbuild test robot wrote:
> > tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> > head: 5453a3df2a5eb49bc24615d4cf0d66b2aae05e5f
> > commit: e9e0c8903009477b630e37a8b6364b26a00720da fanotify: encode file identifier for FAN_REPORT_FID
> > date: 5 weeks ago
> > config: mips-allmodconfig (attached as .config)
> > compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > git checkout e9e0c8903009477b630e37a8b6364b26a00720da
> > # save the attached .config to linux build tree
> > GCC_VERSION=7.2.0 make.cross ARCH=mips
> >
> > All warnings (new ones prefixed by >>):
> >
> > In file included from include/linux/kernel.h:14:0,
> > from include/linux/list.h:9,
> > from include/linux/preempt.h:11,
> > from include/linux/spinlock.h:51,
> > from include/linux/fdtable.h:11,
> > from fs/notify/fanotify/fanotify.c:3:
> > fs/notify/fanotify/fanotify.c: In function 'fanotify_encode_fid':
> > include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 2 has type 'long int' [-Wformat=]
> > #define KERN_SOH "\001" /* ASCII Start Of Header */
> > ^
> > include/linux/printk.h:424:10: note: in definition of macro 'printk_ratelimited'
> > printk(fmt, ##__VA_ARGS__); \
> > ^~~
> > include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
> > #define KERN_WARNING KERN_SOH "4" /* warning conditions */
> > ^~~~~~~~
> > include/linux/printk.h:440:21: note: in expansion of macro 'KERN_WARNING'
> > printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > ^~~~~~~~~~~~
> > >> fs/notify/fanotify/fanotify.c:198:2: note: in expansion of macro 'pr_warn_ratelimited'
> > pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> > ^~~~~~~~~~~~~~~~~~~
> > fs/notify/fanotify/fanotify.c:198:61: note: format string is defined here
> > pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> > ~^
> > %lx
> > In file included from include/linux/kernel.h:14:0,
> > from include/linux/list.h:9,
> > from include/linux/preempt.h:11,
> > from include/linux/spinlock.h:51,
> > from include/linux/fdtable.h:11,
> > from fs/notify/fanotify/fanotify.c:3:
> > include/linux/kern_levels.h:5:18: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'long int' [-Wformat=]
> > #define KERN_SOH "\001" /* ASCII Start Of Header */
> > ^
> > include/linux/printk.h:424:10: note: in definition of macro 'printk_ratelimited'
> > printk(fmt, ##__VA_ARGS__); \
> > ^~~
> > include/linux/kern_levels.h:12:22: note: in expansion of macro 'KERN_SOH'
> > #define KERN_WARNING KERN_SOH "4" /* warning conditions */
> > ^~~~~~~~
> > include/linux/printk.h:440:21: note: in expansion of macro 'KERN_WARNING'
> > printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > ^~~~~~~~~~~~
> > >> fs/notify/fanotify/fanotify.c:198:2: note: in expansion of macro 'pr_warn_ratelimited'
> > pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> > ^~~~~~~~~~~~~~~~~~~
> > fs/notify/fanotify/fanotify.c:198:64: note: format string is defined here
> > pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> > ~^
> > %lx
> >
> > vim +/pr_warn_ratelimited +198 fs/notify/fanotify/fanotify.c
> >
> > 154
> > 155 static int fanotify_encode_fid(struct fanotify_event *event,
> > 156 const struct path *path, gfp_t gfp)
> > 157 {
> > 158 struct fanotify_fid *fid = &event->fid;
> > 159 int dwords, bytes = 0;
> > 160 struct kstatfs stat;
> > 161 int err, type;
> > 162
> > 163 stat.f_fsid.val[0] = stat.f_fsid.val[1] = 0;
> > 164 fid->ext_fh = NULL;
> > 165 dwords = 0;
> > 166 err = -ENOENT;
> > 167 type = exportfs_encode_inode_fh(d_inode(path->dentry), NULL, &dwords,
> > 168 NULL);
> > 169 if (!dwords)
> > 170 goto out_err;
> > 171
> > 172 err = vfs_statfs(path, &stat);
> > 173 if (err)
> > 174 goto out_err;
> > 175
> > 176 bytes = dwords << 2;
> > 177 if (bytes > FANOTIFY_INLINE_FH_LEN) {
> > 178 /* Treat failure to allocate fh as failure to allocate event */
> > 179 err = -ENOMEM;
> > 180 fid->ext_fh = kmalloc(bytes, gfp);
> > 181 if (!fid->ext_fh)
> > 182 goto out_err;
> > 183 }
> > 184
> > 185 type = exportfs_encode_inode_fh(d_inode(path->dentry),
> > 186 fanotify_fid_fh(fid, bytes), &dwords,
> > 187 NULL);
> > 188 err = -EINVAL;
> > 189 if (!type || type == FILEID_INVALID || bytes != dwords << 2)
> > 190 goto out_err;
> > 191
> > 192 fid->fsid = stat.f_fsid;
> > 193 event->fh_len = bytes;
> > 194
> > 195 return type;
> > 196
> > 197 out_err:
> > > 198 pr_warn_ratelimited("fanotify: failed to encode fid (fsid=%x.%x, "
> > 199 "type=%d, bytes=%d, err=%i)\n",
> > 200 stat.f_fsid.val[0], stat.f_fsid.val[1],
> > 201 type, bytes, err);
> > 202 kfree(fid->ext_fh);
> > 203 fid->ext_fh = NULL;
> > 204 event->fh_len = 0;
> > 205
> > 206 return FILEID_INVALID;
> > 207 }
> > 208
> >
> > ---
> > 0-DAY kernel test infrastructure Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
>
> --
> Jan Kara <[email protected]>
> SUSE Labs, CR
On Thu 14-03-19 14:01:18, Amir Goldstein wrote:
> On Thu, Mar 14, 2019 at 10:37 AM Jan Kara <[email protected]> wrote:
> >
> > AFAICS this is the known problem with weird mips definitions of
> > __kernel_fsid_t which uses long whereas all other architectures use int,
> > right? Seeing that mips can actually have 8-byte longs, I guess this
> > bogosity is just wired in the kernel API and we cannot easily fix it in
> > mips (mips guys, correct me if I'm wrong). So what if we just
> > unconditionally typed printed values to unsigned int to silence the
> > warning?
>
> I don't understand why. To me that sounds like papering over a bug.
>
> See this reply from mips developer Paul Burton:
> https://marc.info/?l=linux-fsdevel&m=154783680019904&w=2
> mips developers have not replied to the question why __kernel_fsid_t
> should use long.
Ah, right. I've missed that mips defines __kernel_fsid_t only if
sizeof(long) == 4. OK, than fixing MIPS headers is definitely what we ought
to do. Mips guys, any reason why the patch from Ralf didn't get merged yet?
Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Thu, Mar 14, 2019 at 01:38:11PM +0100, Jan Kara wrote:
> On Thu 14-03-19 14:01:18, Amir Goldstein wrote:
> > On Thu, Mar 14, 2019 at 10:37 AM Jan Kara <[email protected]> wrote:
> > >
> > > AFAICS this is the known problem with weird mips definitions of
> > > __kernel_fsid_t which uses long whereas all other architectures use int,
> > > right? Seeing that mips can actually have 8-byte longs, I guess this
> > > bogosity is just wired in the kernel API and we cannot easily fix it in
> > > mips (mips guys, correct me if I'm wrong). So what if we just
> > > unconditionally typed printed values to unsigned int to silence the
> > > warning?
> >
> > I don't understand why. To me that sounds like papering over a bug.
> >
> > See this reply from mips developer Paul Burton:
> > https://marc.info/?l=linux-fsdevel&m=154783680019904&w=2
> > mips developers have not replied to the question why __kernel_fsid_t
> > should use long.
>
> Ah, right. I've missed that mips defines __kernel_fsid_t only if
> sizeof(long) == 4. OK, than fixing MIPS headers is definitely what we ought
> to do. Mips guys, any reason why the patch from Ralf didn't get merged yet?
Paul's patch :-)
As for the reason why the definition is as it is - 32-bit MIPS was
born using long, then in 2000 64-bit MIPS started off as arch/mips64
using int. Eventually the two ports were combined using:
ypedef struct {
#if (_MIPS_SZLONG == 32)
long val[2];
#endif
#if (_MIPS_SZLONG == 64)
int val[2];
#endif
} __kernel_fsid_t;
A desparate attempt to use asm-generic where ever possible then resulted
in the confusing definition we'e having today.
Normally APIs are cast into stone not to be changed. But fsid is used in
struct statfs and the man page states "Nobody knows what f_fsid is supposed
to contain (but see below)." and f_fsid is supposed to be opaque anyway so
I'm wondering if something could break at all. Researching that.
Ralf
On Thu, Mar 14, 2019 at 4:34 PM Ralf Baechle <[email protected]> wrote:
>
> On Thu, Mar 14, 2019 at 01:38:11PM +0100, Jan Kara wrote:
>
> > On Thu 14-03-19 14:01:18, Amir Goldstein wrote:
> > > On Thu, Mar 14, 2019 at 10:37 AM Jan Kara <[email protected]> wrote:
> > > >
> > > > AFAICS this is the known problem with weird mips definitions of
> > > > __kernel_fsid_t which uses long whereas all other architectures use int,
> > > > right? Seeing that mips can actually have 8-byte longs, I guess this
> > > > bogosity is just wired in the kernel API and we cannot easily fix it in
> > > > mips (mips guys, correct me if I'm wrong). So what if we just
> > > > unconditionally typed printed values to unsigned int to silence the
> > > > warning?
> > >
> > > I don't understand why. To me that sounds like papering over a bug.
> > >
> > > See this reply from mips developer Paul Burton:
> > > https://marc.info/?l=linux-fsdevel&m=154783680019904&w=2
> > > mips developers have not replied to the question why __kernel_fsid_t
> > > should use long.
> >
> > Ah, right. I've missed that mips defines __kernel_fsid_t only if
> > sizeof(long) == 4. OK, than fixing MIPS headers is definitely what we ought
> > to do. Mips guys, any reason why the patch from Ralf didn't get merged yet?
>
> Paul's patch :-)
>
> As for the reason why the definition is as it is - 32-bit MIPS was
> born using long, then in 2000 64-bit MIPS started off as arch/mips64
> using int. Eventually the two ports were combined using:
>
> ypedef struct {
> #if (_MIPS_SZLONG == 32)
> long val[2];
> #endif
> #if (_MIPS_SZLONG == 64)
> int val[2];
> #endif
> } __kernel_fsid_t;
>
> A desparate attempt to use asm-generic where ever possible then resulted
> in the confusing definition we'e having today.
>
> Normally APIs are cast into stone not to be changed. But fsid is used in
> struct statfs and the man page states "Nobody knows what f_fsid is supposed
> to contain (but see below)." and f_fsid is supposed to be opaque anyway so
> I'm wondering if something could break at all. Researching that.
>
Its content is opaque, but its size must be equal to that of fsid_t
from glibc/toolchain headers. Do the mips32 glibc headers also
define fsid_t as long val[2], or do they define it as int val[2]?
Thanks,
Amir.
Hi Amir,
On Thu, Mar 14, 2019 at 06:16:35PM +0200, Amir Goldstein wrote:
> On Thu, Mar 14, 2019 at 4:34 PM Ralf Baechle <[email protected]> wrote:
> > On Thu, Mar 14, 2019 at 01:38:11PM +0100, Jan Kara wrote:
> > > On Thu 14-03-19 14:01:18, Amir Goldstein wrote:
> > > > On Thu, Mar 14, 2019 at 10:37 AM Jan Kara <[email protected]> wrote:
> > > > > AFAICS this is the known problem with weird mips definitions of
> > > > > __kernel_fsid_t which uses long whereas all other architectures use int,
> > > > > right? Seeing that mips can actually have 8-byte longs, I guess this
> > > > > bogosity is just wired in the kernel API and we cannot easily fix it in
> > > > > mips (mips guys, correct me if I'm wrong). So what if we just
> > > > > unconditionally typed printed values to unsigned int to silence the
> > > > > warning?
> > > >
> > > > I don't understand why. To me that sounds like papering over a bug.
> > > >
> > > > See this reply from mips developer Paul Burton:
> > > > https://marc.info/?l=linux-fsdevel&m=154783680019904&w=2
> > > > mips developers have not replied to the question why __kernel_fsid_t
> > > > should use long.
> > >
> > > Ah, right. I've missed that mips defines __kernel_fsid_t only if
> > > sizeof(long) == 4. OK, than fixing MIPS headers is definitely what we ought
> > > to do. Mips guys, any reason why the patch from Ralf didn't get merged yet?
> >
> > Paul's patch :-)
> >
> > As for the reason why the definition is as it is - 32-bit MIPS was
> > born using long, then in 2000 64-bit MIPS started off as arch/mips64
> > using int. Eventually the two ports were combined using:
> >
> > ypedef struct {
> > #if (_MIPS_SZLONG == 32)
> > long val[2];
> > #endif
> > #if (_MIPS_SZLONG == 64)
> > int val[2];
> > #endif
> > } __kernel_fsid_t;
> >
> > A desparate attempt to use asm-generic where ever possible then resulted
> > in the confusing definition we'e having today.
> >
> > Normally APIs are cast into stone not to be changed. But fsid is used in
> > struct statfs and the man page states "Nobody knows what f_fsid is supposed
> > to contain (but see below)." and f_fsid is supposed to be opaque anyway so
> > I'm wondering if something could break at all. Researching that.
> >
>
> Its content is opaque, but its size must be equal to that of fsid_t
> from glibc/toolchain headers. Do the mips32 glibc headers also
> define fsid_t as long val[2], or do they define it as int val[2]?
First off, my apologies that my proposed patch slipped through the
cracks. It somehow didn't make it onto my to-do list & after that there
was little chance I was going to remember it until someone replied... :)
I've just polished off the patch & submitted it [1]. Presuming nobody
has a problem with it in the next couple of days, I'll apply it to
mips-fixes & send it on to Linus.
To address your question about glibc headers - it shouldn't matter. On
MIPS32 int & long are the same, so even if userland & the kernel
disagree about the type the data in memory should be identical.
[1] https://lore.kernel.org/linux-mips/[email protected]/T/#u
Thanks,
Paul