Where KASASERT fd < fdp->fd_nfiles should be?

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Where KASASERT fd < fdp->fd_nfiles should be?

Mariusz Zaborski
Some time ago mjg@ had an idea to cleanup use of the fget_locked function in
the sys_capability. I implemented most of it and pjd@ accepted almost all
changes (with one suggestion with I didn't figure out what to do with it).

In my patch I remove one KASSERT from the cap_ioctl_check:
int
cap_ioctl_check(struct filedesc *fdp, int fd, u_long cmd)
{
        u_long *cmds;
        ssize_t ncmds;
        long i;

        FILEDESC_LOCK_ASSERT(fdp);
        KASSERT(fd >= 0 && fd < fdp->fd_nfiles,
            ("%s: invalid fd=%d", __func__, fd));

        ncmds = fdp->fd_ofiles[fd].fde_nioctls;
        if (ncmds == -1)
                return (0);

        cmds = fdp->fd_ofiles[fd].fde_ioctls;
        for (i = 0; i < ncmds; i++) {
                if (cmds[i] == cmd)
                        return (0);
        }

        return (ENOTCAPABLE);
}

My question and problem is do we need this KASSERT?
The fdget_locked checks if the fd is not larger then fd_lastfile.
But the code from fdinit suggest that fd_lastfile can be larger then fd_nfiles.
pjd@ suggested that it can go over size of the table fd_ofiles array:
        while (fdp->fd_lastfile >= newfdp->fd_nfiles) {
                FILEDESC_SUNLOCK(fdp);
                fdgrowtable(newfdp, fdp->fd_lastfile + 1);
                FILEDESC_SLOCK(fdp);
        }

So the question is do we need this assertion here or maybe should we move it to
the fget_locked()/fdget_locked() functions?

Thanks,
--
Mariusz Zaborski
oshogbo//vx | http://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http://wheelsystems.com
If it's not broken, let's fix it till it is!!1

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Where KASASERT fd < fdp->fd_nfiles should be?

Mateusz Guzik
On Sat, Feb 17, 2018 at 04:02:24PM +0100, Mariusz Zaborski wrote:
>
>         KASSERT(fd >= 0 && fd < fdp->fd_nfiles,
>             ("%s: invalid fd=%d", __func__, fd));
>
>
> My question and problem is do we need this KASSERT?
> The fdget_locked checks if the fd is not larger then fd_lastfile.
> But the code from fdinit suggest that fd_lastfile can be larger then
fd_nfiles.
> pjd@ suggested that it can go over size of the table fd_ofiles array:
>         while (fdp->fd_lastfile >= newfdp->fd_nfiles) {
>                 FILEDESC_SUNLOCK(fdp);
>                 fdgrowtable(newfdp, fdp->fd_lastfile + 1);
>                 FILEDESC_SLOCK(fdp);
>         }
>
> So the question is do we need this assertion here or maybe should we move
it to
> the fget_locked()/fdget_locked() functions?
>

While the assertion arguably can be removed, it is most definitely
valid.

fd_nfiles signifies the size of the table, while fd_lastfile the highest
used slot. By definition it must fit the table.

The code sample is used on fork where the existing table is duplicated.
Allocation of the new table is performed with locks dropped, which means
the old one can grow in the meantime. The while loop ensures the new
table will have the right size no matter what.

--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: Where KASASERT fd < fdp->fd_nfiles should be?

Mariusz Zaborski
On Sat, Feb 17, 2018 at 04:23:40PM +0100, Mateusz Guzik wrote:

> On Sat, Feb 17, 2018 at 04:02:24PM +0100, Mariusz Zaborski wrote:
> >
> >         KASSERT(fd >= 0 && fd < fdp->fd_nfiles,
> >             ("%s: invalid fd=%d", __func__, fd));
> >
> >
> > My question and problem is do we need this KASSERT?
> > The fdget_locked checks if the fd is not larger then fd_lastfile.
> > But the code from fdinit suggest that fd_lastfile can be larger then
> fd_nfiles.
> > pjd@ suggested that it can go over size of the table fd_ofiles array:
> >         while (fdp->fd_lastfile >= newfdp->fd_nfiles) {
> >                 FILEDESC_SUNLOCK(fdp);
> >                 fdgrowtable(newfdp, fdp->fd_lastfile + 1);
> >                 FILEDESC_SLOCK(fdp);
> >         }
> >
> > So the question is do we need this assertion here or maybe should we move
> it to
> > the fget_locked()/fdget_locked() functions?
> >
>
> While the assertion arguably can be removed, it is most definitely
> valid.
>
> fd_nfiles signifies the size of the table, while fd_lastfile the highest
> used slot. By definition it must fit the table.
>
> The code sample is used on fork where the existing table is duplicated.
> Allocation of the new table is performed with locks dropped, which means
> the old one can grow in the meantime. The while loop ensures the new
> table will have the right size no matter what.
So maybe we should go in more general approach and move this KASSERT to the
fget_locked and fdget_locked functions?

Thanks,
--
Mariusz Zaborski
oshogbo//vx | http://oshogbo.vexillium.org
FreeBSD commiter | https://freebsd.org
Software developer | http://wheelsystems.com
If it's not broken, let's fix it till it is!!1

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Where KASASERT fd < fdp->fd_nfiles should be?

Mateusz Guzik
On Sat, Feb 17, 2018 at 4:38 PM, Mariusz Zaborski <[hidden email]>
wrote:

> So maybe we should go in more general approach and move this KASSERT to the
> fget_locked and fdget_locked functions?
>
>
If anyone is going to do anything with fd_lastfile, the right course of
action is to whack it.

A soft prerequisite is that someone takes care of the current
hand-rolled bitmap implementation.

--
Mateusz Guzik <mjguzik gmail.com>
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-hackers
To unsubscribe, send any mail to "[hidden email]"