* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Konstantin Khlebnikov @ 2015-08-25 13:58 UTC (permalink / raw)
To: Michal Hocko
Cc: Eric B Munson, Andrew Morton, Vlastimil Babka, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <20150825134154.GB6285-2MMpYkNvuYDjFM9bn6wA6Q@public.gmane.org>
On Tue, Aug 25, 2015 at 4:41 PM, Michal Hocko <mhocko-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Fri 21-08-15 14:31:32, Eric B Munson wrote:
> [...]
>> I am in the middle of implementing lock on fault this way, but I cannot
>> see how we will hanlde mremap of a lock on fault region. Say we have
>> the following:
>>
>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> mlock(addr, len, MLOCK_ONFAULT);
>> ...
>> mremap(addr, len, 2 * len, ...)
>>
>> There is no way for mremap to know that the area being remapped was lock
>> on fault so it will be locked and prefaulted by remap. How can we avoid
>> this without tracking per vma if it was locked with lock or lock on
>> fault?
>
> Yes mremap is a problem and it is very much similar to mmap(MAP_LOCKED).
> It doesn't guarantee the full mlock semantic because it leaves partially
> populated ranges behind without reporting any error.
>
> Considering the current behavior I do not thing it would be terrible
> thing to do what Konstantin was suggesting and populate only the full
> ranges in a best effort mode (it is done so anyway) and document the
> behavior properly.
> "
> If the memory segment specified by old_address and old_size is
> locked (using mlock(2) or similar), then this lock is maintained
> when the segment is resized and/or relocated. As a consequence,
> the amount of memory locked by the process may change.
>
> If the range is already fully populated and the range is
> enlarged the new range is attempted to be fully populated
> as well to preserve the full mlock semantic but there is no
> guarantee this will succeed. Partially populated (e.g. created by
> mlock(MLOCK_ONFAULT)) ranges do not have the full mlock semantic
> so they are not populated on resize.
> "
>
> So what we have as a result is that partially populated ranges are
> preserved and fully populated ones work in the best effort mode the same
> way as they are now.
>
> Does that sound at least remotely reasonably?
The problem is that mremap have to scan ptes to detect that and old behaviour
becomes very fragile: one fail and mremap will never populate that vma again.
For now I think new flag "MREMAP_NOPOPULATE" is a better option.
>
>
> --
> Michal Hocko
> SUSE Labs
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo-Bw31MaZKKs0EbZ0PF+XxCw@public.gmane.org For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org"> email-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org </a>
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Vlastimil Babka @ 2015-08-25 13:55 UTC (permalink / raw)
To: Michal Hocko, Eric B Munson
Cc: Andrew Morton, Jonathan Corbet, Kirill A. Shutemov, linux-kernel,
dri-devel, linux-mm, linux-api
In-Reply-To: <20150825134154.GB6285@dhcp22.suse.cz>
On 08/25/2015 03:41 PM, Michal Hocko wrote:
> On Fri 21-08-15 14:31:32, Eric B Munson wrote:
> [...]
>> I am in the middle of implementing lock on fault this way, but I cannot
>> see how we will hanlde mremap of a lock on fault region. Say we have
>> the following:
>>
>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> mlock(addr, len, MLOCK_ONFAULT);
>> ...
>> mremap(addr, len, 2 * len, ...)
>>
>> There is no way for mremap to know that the area being remapped was lock
>> on fault so it will be locked and prefaulted by remap. How can we avoid
>> this without tracking per vma if it was locked with lock or lock on
>> fault?
>
> Yes mremap is a problem and it is very much similar to mmap(MAP_LOCKED).
> It doesn't guarantee the full mlock semantic because it leaves partially
> populated ranges behind without reporting any error.
Hm, that's right.
> Considering the current behavior I do not thing it would be terrible
> thing to do what Konstantin was suggesting and populate only the full
> ranges in a best effort mode (it is done so anyway) and document the
> behavior properly.
> "
> If the memory segment specified by old_address and old_size is
> locked (using mlock(2) or similar), then this lock is maintained
> when the segment is resized and/or relocated. As a consequence,
> the amount of memory locked by the process may change.
>
> If the range is already fully populated and the range is
> enlarged the new range is attempted to be fully populated
> as well to preserve the full mlock semantic but there is no
> guarantee this will succeed. Partially populated (e.g. created by
> mlock(MLOCK_ONFAULT)) ranges do not have the full mlock semantic
> so they are not populated on resize.
> "
>
> So what we have as a result is that partially populated ranges are
> preserved and fully populated ones work in the best effort mode the same
> way as they are now.
>
> Does that sound at least remotely reasonably?
I'll basically repeat what I said earlier:
- mremap scanning existing pte's to figure out the population would slow
it down for no good reason
- it would be unreliable anyway:
- example: was the area completely populated because MLOCK_ONFAULT
was not used or because the process faulted it already
- example: was the area not completely populated because
MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to populate
it fully?
I think the first point is a pointless regression for workloads that use
just plain mlock() and don't want the onfault semantics. Unless there's
some shortcut? Does vma have a counter of how much is populated? (I
don't think so?)
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Michal Hocko @ 2015-08-25 13:41 UTC (permalink / raw)
To: Eric B Munson
Cc: Andrew Morton, Vlastimil Babka, Jonathan Corbet,
Kirill A. Shutemov, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150821183132.GA12835-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
On Fri 21-08-15 14:31:32, Eric B Munson wrote:
[...]
> I am in the middle of implementing lock on fault this way, but I cannot
> see how we will hanlde mremap of a lock on fault region. Say we have
> the following:
>
> addr = mmap(len, MAP_ANONYMOUS, ...);
> mlock(addr, len, MLOCK_ONFAULT);
> ...
> mremap(addr, len, 2 * len, ...)
>
> There is no way for mremap to know that the area being remapped was lock
> on fault so it will be locked and prefaulted by remap. How can we avoid
> this without tracking per vma if it was locked with lock or lock on
> fault?
Yes mremap is a problem and it is very much similar to mmap(MAP_LOCKED).
It doesn't guarantee the full mlock semantic because it leaves partially
populated ranges behind without reporting any error.
Considering the current behavior I do not thing it would be terrible
thing to do what Konstantin was suggesting and populate only the full
ranges in a best effort mode (it is done so anyway) and document the
behavior properly.
"
If the memory segment specified by old_address and old_size is
locked (using mlock(2) or similar), then this lock is maintained
when the segment is resized and/or relocated. As a consequence,
the amount of memory locked by the process may change.
If the range is already fully populated and the range is
enlarged the new range is attempted to be fully populated
as well to preserve the full mlock semantic but there is no
guarantee this will succeed. Partially populated (e.g. created by
mlock(MLOCK_ONFAULT)) ranges do not have the full mlock semantic
so they are not populated on resize.
"
So what we have as a result is that partially populated ranges are
preserved and fully populated ones work in the best effort mode the same
way as they are now.
Does that sound at least remotely reasonably?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v7 39/44] [media] uapi/media.h: Add MEDIA_IOC_G_TOPOLOGY ioctl
From: Mauro Carvalho Chehab @ 2015-08-25 11:36 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Mauro Carvalho Chehab,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <55DC366C.5050509-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>
Em Tue, 25 Aug 2015 11:33:32 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> escreveu:
> On 08/23/15 22:17, Mauro Carvalho Chehab wrote:
> > Add a new ioctl that will report the entire topology on
> > one go.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> >
> > diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> > index 796e4a490af8..0111d9652b78 100644
> > --- a/include/media/media-entity.h
> > +++ b/include/media/media-entity.h
> > @@ -181,6 +181,8 @@ struct media_interface {
> > */
> > struct media_intf_devnode {
> > struct media_interface intf;
> > +
> > + /* Should match the fields at media_v2_intf_devnode */
> > u32 major;
> > u32 minor;
> > };
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index ceea791dd6e9..7fcf7f477ae3 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -238,11 +238,94 @@ struct media_links_enum {
> > #define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4)
> > #define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)
> >
> > -/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
> > +/*
> > + * MC next gen API definitions
> > + *
> > + * NOTE: The declarations below are close to the MC RFC for the Media
> > + * Controller, the next generation. Yet, there are a few adjustments
> > + * to do, as we want to be able to have a functional API before
> > + * the MC properties change. Those will be properly marked below.
> > + * Please also notice that I removed "num_pads", "num_links",
> > + * from the proposal, as a proper userspace application will likely
> > + * use lists for pads/links, just as we intend todo in Kernelspace.
> > + * The API definition should be freed from fields that are bound to
> > + * some specific data structure.
> > + *
> > + * FIXME: Currently, I opted to name the new types as "media_v2", as this
> > + * won't cause any conflict with the Kernelspace namespace, nor with
> > + * the previous kAPI media_*_desc namespace. This can be changed
> > + * latter, before the adding this API upstream.
> > + */
> > +
> > +
> > +#define MEDIA_NEW_LNK_FL_ENABLED MEDIA_LNK_FL_ENABLED
> > +#define MEDIA_NEW_LNK_FL_IMMUTABLE MEDIA_LNK_FL_IMMUTABLE
> > +#define MEDIA_NEW_LNK_FL_DYNAMIC MEDIA_NEW_FL_DYNAMIC
> > +#define MEDIA_NEW_LNK_FL_INTERFACE_LINK (1 << 3)
> > +
> > +struct media_v2_entity {
> > + __u32 id;
> > + char name[64]; /* FIXME: move to a property? (RFC says so) */
> > + __u16 reserved[14];
> > +};
> > +
> > +/* Should match the specific fields at media_intf_devnode */
> > +struct media_v2_intf_devnode {
> > + __u32 major;
> > + __u32 minor;
> > +};
> > +
> > +struct media_v2_interface {
> > + __u32 id;
> > + __u32 intf_type;
> > + __u32 flags;
> > + __u32 reserved[9];
> > +
> > + union {
> > + struct media_v2_intf_devnode devnode;
> > + __u32 raw[16];
> > + };
> > +};
> > +
> > +struct media_v2_pad {
> > + __u32 id;
> > + __u32 entity_id;
> > + __u32 flags;
> > + __u16 reserved[9];
> > +};
> > +
> > +struct media_v2_link {
> > + __u32 id;
> > + __u32 source_id;
> > + __u32 sink_id;
> > + __u32 flags;
> > + __u32 reserved[5];
> > +};
> > +
> > +struct media_v2_topology {
> > + __u32 topology_version;
> > +
> > + __u32 num_entities;
> > + struct media_v2_entity *entities;
> > +
> > + __u32 num_interfaces;
> > + struct media_v2_interface *interfaces;
> > +
> > + __u32 num_pads;
> > + struct media_v2_pad *pads;
> > +
> > + __u32 num_links;
> > + struct media_v2_link *links;
> > +
> > + __u32 reserved[64];
>
> As I suggested elsewhere, replace this by:
>
> struct {
> __u32 num_reserved;
> void *ptr_reserved;
> } reserved_ptrs[8];
>
> This will keep the number of reserved num/pointer pairs identical
> between 32 and 64 bit architectures. Without that doing compat32
> handling will be very difficult indeed.
>
> We might want a separate __u32 reserved[] array so we're able to add
> non-pointer fields in the future.
OK.
>
> Regards,
>
> Hans
>
> > +};
> > +
> > +/* ioctls */
> >
> > #define MEDIA_IOC_DEVICE_INFO _IOWR('|', 0x00, struct media_device_info)
> > #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct media_entity_desc)
> > #define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
> > #define MEDIA_IOC_SETUP_LINK _IOWR('|', 0x03, struct media_link_desc)
> > +#define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
> >
> > #endif /* __LINUX_MEDIA_H */
> >
^ permalink raw reply
* Re: [PATCH v7 24/44] [media] uapi/media.h: Fix entity namespace
From: Mauro Carvalho Chehab @ 2015-08-25 11:25 UTC (permalink / raw)
To: Hans Verkuil
Cc: Linux Media Mailing List, Mauro Carvalho Chehab, Hans Verkuil,
Laurent Pinchart, Sakari Ailus, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <55DC2E2D.4090000-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org>
Em Tue, 25 Aug 2015 10:58:21 +0200
Hans Verkuil <hverkuil-qWit8jRvyhVmR6Xm/wNWPw@public.gmane.org> escreveu:
> On 08/23/15 22:17, Mauro Carvalho Chehab wrote:
> > Now that interfaces got created, we need to fix the entity
> > namespace.
> >
> > So, let's create a consistent new namespace and add backward
> > compatibility macros to keep the old namespace preserved.
> >
> > Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
> >
> > diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> > index 5a2bd03f5dc0..acada5ba9442 100644
> > --- a/drivers/media/dvb-core/dvbdev.c
> > +++ b/drivers/media/dvb-core/dvbdev.c
> > @@ -229,17 +229,17 @@ static void dvb_create_media_entity(struct dvb_device *dvbdev,
> >
> > switch (type) {
> > case DVB_DEVICE_FRONTEND:
> > - dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_FE;
> > + dvbdev->entity->type = MEDIA_ENT_T_DVB_DEMOD;
> > dvbdev->pads[0].flags = MEDIA_PAD_FL_SINK;
> > dvbdev->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> > break;
> > case DVB_DEVICE_DEMUX:
> > - dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_DEMUX;
> > + dvbdev->entity->type = MEDIA_ENT_T_DVB_DEMUX;
> > dvbdev->pads[0].flags = MEDIA_PAD_FL_SINK;
> > dvbdev->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> > break;
> > case DVB_DEVICE_CA:
> > - dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_CA;
> > + dvbdev->entity->type = MEDIA_ENT_T_DVB_CA;
> > dvbdev->pads[0].flags = MEDIA_PAD_FL_SINK;
> > dvbdev->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> > break;
> > @@ -438,7 +438,7 @@ EXPORT_SYMBOL(dvb_unregister_device);
> > void dvb_create_media_graph(struct dvb_adapter *adap)
> > {
> > struct media_device *mdev = adap->mdev;
> > - struct media_entity *entity, *tuner = NULL, *fe = NULL;
> > + struct media_entity *entity, *tuner = NULL, *demod = NULL;
> > struct media_entity *demux = NULL, *dvr = NULL, *ca = NULL;
> > struct media_interface *intf;
> >
> > @@ -450,26 +450,26 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
> > case MEDIA_ENT_T_V4L2_SUBDEV_TUNER:
> > tuner = entity;
> > break;
> > - case MEDIA_ENT_T_DEVNODE_DVB_FE:
> > - fe = entity;
> > + case MEDIA_ENT_T_DVB_DEMOD:
> > + demod = entity;
> > break;
> > - case MEDIA_ENT_T_DEVNODE_DVB_DEMUX:
> > + case MEDIA_ENT_T_DVB_DEMUX:
> > demux = entity;
> > break;
> > - case MEDIA_ENT_T_DEVNODE_DVB_DVR:
> > + case MEDIA_ENT_T_DVB_TSOUT:
> > dvr = entity;
> > break;
> > - case MEDIA_ENT_T_DEVNODE_DVB_CA:
> > + case MEDIA_ENT_T_DVB_CA:
> > ca = entity;
> > break;
> > }
> > }
> >
> > - if (tuner && fe)
> > - media_create_pad_link(tuner, 0, fe, 0, 0);
> > + if (tuner && demod)
> > + media_create_pad_link(tuner, 0, demod, 0, 0);
> >
> > - if (fe && demux)
> > - media_create_pad_link(fe, 1, demux, 0, MEDIA_LNK_FL_ENABLED);
> > + if (demod && demux)
> > + media_create_pad_link(demod, 1, demux, 0, MEDIA_LNK_FL_ENABLED);
> >
> > if (demux && dvr)
> > media_create_pad_link(demux, 1, dvr, 0, MEDIA_LNK_FL_ENABLED);
> > diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> > index 21c96cd7a6ae..7306aeaff807 100644
> > --- a/include/uapi/linux/media.h
> > +++ b/include/uapi/linux/media.h
> > @@ -42,31 +42,67 @@ struct media_device_info {
> >
> > #define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
> >
> > +/*
> > + * Base numbers for entity types
> > + *
> > + * Please notice that the huge gap of 16 bits for each base is overkill!
> > + * 8 bits is more than enough to avoid starving entity types for each
> > + * subsystem.
> > + *
> > + * However, It is kept this way just to avoid binary breakages with the
> > + * namespace provided on legacy versions of this header.
> > + */
> > +#define MEDIA_ENT_T_DVB_BASE 0x00000000
> > +#define MEDIA_ENT_T_V4L2_BASE 0x00010000
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_BASE 0x00020000
> > +
> > +/* V4L2 entities */
> > +#define MEDIA_ENT_T_V4L2_VIDEO (MEDIA_ENT_T_V4L2_BASE + 1)
> > + /*
> > + * Please notice that numbers between MEDIA_ENT_T_V4L2_BASE + 2 and
> > + * MEDIA_ENT_T_V4L2_BASE + 4 can't be used, as those values used
> > + * to be declared for FB, ALSA and DVB entities.
> > + * As those values were never atually used in practice, we're just
>
> s/atually/actually/
>
> > + * adding them as backward compatibily macros and keeping the
>
> s/compatibily/compatibility/
>
> > + * numberspace cleaned here. This way, we avoid breaking compilation,
>
> s/cleaned/clean/
>
> > + * in the case of having some userspace application using the old
> > + * symbols.
> > + */
> > +#define MEDIA_ENT_T_V4L2_VBI (MEDIA_ENT_T_V4L2_BASE + 5)
> > +#define MEDIA_ENT_T_V4L2_RADIO (MEDIA_ENT_T_V4L2_BASE + 6)
> > +#define MEDIA_ENT_T_V4L2_SWRADIO (MEDIA_ENT_T_V4L2_BASE + 7)
>
> Why are these entities? Aren't these interface types?
We need both:
The entity represents the data sinks (or sources), and the interface the
control interfaces.
I'm actually in doubt about MEDIA_ENT_T_V4L2_RADIO. Maybe we'll need a
MEDIA_ENT_T_V4L2_RADIO_RDS too.
Of course, MEDIA_ENT_T_V4L2_RADIO is not needed for receivers, but I
guess it is needed for TX.
>
> > +
> > +/* V4L2 Sub-device entities */
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1)
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2)
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_LENS (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3)
> > + /* A converter of analogue video to its digital representation. */
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4)
> > + /* Tuner entity is actually both V4L2 and DVB subdev */
> > +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5)
> > +
> > +/* DVB entities */
> > +#define MEDIA_ENT_T_DVB_DEMOD (MEDIA_ENT_T_DVB_BASE + 4)
> > +#define MEDIA_ENT_T_DVB_DEMUX (MEDIA_ENT_T_DVB_BASE + 5)
> > +#define MEDIA_ENT_T_DVB_TSOUT (MEDIA_ENT_T_DVB_BASE + 6)
> > +#define MEDIA_ENT_T_DVB_CA (MEDIA_ENT_T_DVB_BASE + 7)
> > +#define MEDIA_ENT_T_DVB_NET_DECAP (MEDIA_ENT_T_DVB_BASE + 8)
> > +
> > +/* Legacy symbols used to avoid userspace compilation breakages */
> > #define MEDIA_ENT_TYPE_SHIFT 16
> > #define MEDIA_ENT_TYPE_MASK 0x00ff0000
> > #define MEDIA_ENT_SUBTYPE_MASK 0x0000ffff
> >
> > -#define MEDIA_ENT_T_DEVNODE (1 << MEDIA_ENT_TYPE_SHIFT)
> > -#define MEDIA_ENT_T_DEVNODE_V4L (MEDIA_ENT_T_DEVNODE + 1)
> > +#define MEDIA_ENT_T_DEVNODE MEDIA_ENT_T_V4L2_BASE
> > +#define MEDIA_ENT_T_V4L2_SUBDEV MEDIA_ENT_T_V4L2_SUBDEV_BASE
> > +
> > +#define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_T_V4L2_VIDEO
> > +
> > #define MEDIA_ENT_T_DEVNODE_FB (MEDIA_ENT_T_DEVNODE + 2)
> > #define MEDIA_ENT_T_DEVNODE_ALSA (MEDIA_ENT_T_DEVNODE + 3)
> > -#define MEDIA_ENT_T_DEVNODE_DVB_FE (MEDIA_ENT_T_DEVNODE + 4)
> > -#define MEDIA_ENT_T_DEVNODE_DVB_DEMUX (MEDIA_ENT_T_DEVNODE + 5)
> > -#define MEDIA_ENT_T_DEVNODE_DVB_DVR (MEDIA_ENT_T_DEVNODE + 6)
> > -#define MEDIA_ENT_T_DEVNODE_DVB_CA (MEDIA_ENT_T_DEVNODE + 7)
> > -#define MEDIA_ENT_T_DEVNODE_DVB_NET (MEDIA_ENT_T_DEVNODE + 8)
> > +#define MEDIA_ENT_T_DEVNODE_DVB (MEDIA_ENT_T_DEVNODE + 4)
> >
> > -/* Legacy symbol. Use it to avoid userspace compilation breakages */
> > -#define MEDIA_ENT_T_DEVNODE_DVB MEDIA_ENT_T_DEVNODE_DVB_FE
> > -
> > -#define MEDIA_ENT_T_V4L2_SUBDEV (2 << MEDIA_ENT_TYPE_SHIFT)
> > -#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV + 1)
> > -#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH (MEDIA_ENT_T_V4L2_SUBDEV + 2)
> > -#define MEDIA_ENT_T_V4L2_SUBDEV_LENS (MEDIA_ENT_T_V4L2_SUBDEV + 3)
> > -/* A converter of analogue video to its digital representation. */
> > -#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV + 4)
> > -
> > -#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV + 5)
> > +/* Entity types */
> >
> > #define MEDIA_ENT_FL_DEFAULT (1 << 0)
> >
> >
>
> Hmm, I'm postponing further review. It might become clearer after reviewing
> more of this patch series.
>
> One reason why this is a bit difficult to review is that it is not immediately
> obvious which defines are here for backwards compat (and shouldn't be used in
> the kernel anymore) and which defines are new.
The ones under /* Legacy symbols used to avoid userspace compilation breakages */
comment should not be used anymore.
> May I suggest that either in this or in a later patch the defines that shouldn't
> be used in the kernel should be placed under #ifndef __KERNEL__?
They'll be under a #ifndef __KERNEL__, but this will happen latter at
the patch series. We need first to remove their usage internally before
adding the ifndef.
Regards,
Mauro
^ permalink raw reply
* Re: [PATCH v7 39/44] [media] uapi/media.h: Add MEDIA_IOC_G_TOPOLOGY ioctl
From: Hans Verkuil @ 2015-08-25 9:33 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Media Mailing List
Cc: Mauro Carvalho Chehab, linux-api
In-Reply-To: <31b28b78f6a37ca7ff4554207bb05cd1a1db788c.1440359643.git.mchehab@osg.samsung.com>
On 08/23/15 22:17, Mauro Carvalho Chehab wrote:
> Add a new ioctl that will report the entire topology on
> one go.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> diff --git a/include/media/media-entity.h b/include/media/media-entity.h
> index 796e4a490af8..0111d9652b78 100644
> --- a/include/media/media-entity.h
> +++ b/include/media/media-entity.h
> @@ -181,6 +181,8 @@ struct media_interface {
> */
> struct media_intf_devnode {
> struct media_interface intf;
> +
> + /* Should match the fields at media_v2_intf_devnode */
> u32 major;
> u32 minor;
> };
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index ceea791dd6e9..7fcf7f477ae3 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -238,11 +238,94 @@ struct media_links_enum {
> #define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4)
> #define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)
>
> -/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
> +/*
> + * MC next gen API definitions
> + *
> + * NOTE: The declarations below are close to the MC RFC for the Media
> + * Controller, the next generation. Yet, there are a few adjustments
> + * to do, as we want to be able to have a functional API before
> + * the MC properties change. Those will be properly marked below.
> + * Please also notice that I removed "num_pads", "num_links",
> + * from the proposal, as a proper userspace application will likely
> + * use lists for pads/links, just as we intend todo in Kernelspace.
> + * The API definition should be freed from fields that are bound to
> + * some specific data structure.
> + *
> + * FIXME: Currently, I opted to name the new types as "media_v2", as this
> + * won't cause any conflict with the Kernelspace namespace, nor with
> + * the previous kAPI media_*_desc namespace. This can be changed
> + * latter, before the adding this API upstream.
> + */
> +
> +
> +#define MEDIA_NEW_LNK_FL_ENABLED MEDIA_LNK_FL_ENABLED
> +#define MEDIA_NEW_LNK_FL_IMMUTABLE MEDIA_LNK_FL_IMMUTABLE
> +#define MEDIA_NEW_LNK_FL_DYNAMIC MEDIA_NEW_FL_DYNAMIC
> +#define MEDIA_NEW_LNK_FL_INTERFACE_LINK (1 << 3)
> +
> +struct media_v2_entity {
> + __u32 id;
> + char name[64]; /* FIXME: move to a property? (RFC says so) */
> + __u16 reserved[14];
> +};
> +
> +/* Should match the specific fields at media_intf_devnode */
> +struct media_v2_intf_devnode {
> + __u32 major;
> + __u32 minor;
> +};
> +
> +struct media_v2_interface {
> + __u32 id;
> + __u32 intf_type;
> + __u32 flags;
> + __u32 reserved[9];
> +
> + union {
> + struct media_v2_intf_devnode devnode;
> + __u32 raw[16];
> + };
> +};
> +
> +struct media_v2_pad {
> + __u32 id;
> + __u32 entity_id;
> + __u32 flags;
> + __u16 reserved[9];
> +};
> +
> +struct media_v2_link {
> + __u32 id;
> + __u32 source_id;
> + __u32 sink_id;
> + __u32 flags;
> + __u32 reserved[5];
> +};
> +
> +struct media_v2_topology {
> + __u32 topology_version;
> +
> + __u32 num_entities;
> + struct media_v2_entity *entities;
> +
> + __u32 num_interfaces;
> + struct media_v2_interface *interfaces;
> +
> + __u32 num_pads;
> + struct media_v2_pad *pads;
> +
> + __u32 num_links;
> + struct media_v2_link *links;
> +
> + __u32 reserved[64];
As I suggested elsewhere, replace this by:
struct {
__u32 num_reserved;
void *ptr_reserved;
} reserved_ptrs[8];
This will keep the number of reserved num/pointer pairs identical
between 32 and 64 bit architectures. Without that doing compat32
handling will be very difficult indeed.
We might want a separate __u32 reserved[] array so we're able to add
non-pointer fields in the future.
Regards,
Hans
> +};
> +
> +/* ioctls */
>
> #define MEDIA_IOC_DEVICE_INFO _IOWR('|', 0x00, struct media_device_info)
> #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct media_entity_desc)
> #define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
> #define MEDIA_IOC_SETUP_LINK _IOWR('|', 0x03, struct media_link_desc)
> +#define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
>
> #endif /* __LINUX_MEDIA_H */
>
^ permalink raw reply
* Re: [PATCH v7 24/44] [media] uapi/media.h: Fix entity namespace
From: Hans Verkuil @ 2015-08-25 8:58 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Hans Verkuil, Laurent Pinchart,
Sakari Ailus, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <5cf25be2d0508e02f6ffe469509fa12c45ddcb8d.1440359643.git.mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
On 08/23/15 22:17, Mauro Carvalho Chehab wrote:
> Now that interfaces got created, we need to fix the entity
> namespace.
>
> So, let's create a consistent new namespace and add backward
> compatibility macros to keep the old namespace preserved.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>
> diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c
> index 5a2bd03f5dc0..acada5ba9442 100644
> --- a/drivers/media/dvb-core/dvbdev.c
> +++ b/drivers/media/dvb-core/dvbdev.c
> @@ -229,17 +229,17 @@ static void dvb_create_media_entity(struct dvb_device *dvbdev,
>
> switch (type) {
> case DVB_DEVICE_FRONTEND:
> - dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_FE;
> + dvbdev->entity->type = MEDIA_ENT_T_DVB_DEMOD;
> dvbdev->pads[0].flags = MEDIA_PAD_FL_SINK;
> dvbdev->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> break;
> case DVB_DEVICE_DEMUX:
> - dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_DEMUX;
> + dvbdev->entity->type = MEDIA_ENT_T_DVB_DEMUX;
> dvbdev->pads[0].flags = MEDIA_PAD_FL_SINK;
> dvbdev->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> break;
> case DVB_DEVICE_CA:
> - dvbdev->entity->type = MEDIA_ENT_T_DEVNODE_DVB_CA;
> + dvbdev->entity->type = MEDIA_ENT_T_DVB_CA;
> dvbdev->pads[0].flags = MEDIA_PAD_FL_SINK;
> dvbdev->pads[1].flags = MEDIA_PAD_FL_SOURCE;
> break;
> @@ -438,7 +438,7 @@ EXPORT_SYMBOL(dvb_unregister_device);
> void dvb_create_media_graph(struct dvb_adapter *adap)
> {
> struct media_device *mdev = adap->mdev;
> - struct media_entity *entity, *tuner = NULL, *fe = NULL;
> + struct media_entity *entity, *tuner = NULL, *demod = NULL;
> struct media_entity *demux = NULL, *dvr = NULL, *ca = NULL;
> struct media_interface *intf;
>
> @@ -450,26 +450,26 @@ void dvb_create_media_graph(struct dvb_adapter *adap)
> case MEDIA_ENT_T_V4L2_SUBDEV_TUNER:
> tuner = entity;
> break;
> - case MEDIA_ENT_T_DEVNODE_DVB_FE:
> - fe = entity;
> + case MEDIA_ENT_T_DVB_DEMOD:
> + demod = entity;
> break;
> - case MEDIA_ENT_T_DEVNODE_DVB_DEMUX:
> + case MEDIA_ENT_T_DVB_DEMUX:
> demux = entity;
> break;
> - case MEDIA_ENT_T_DEVNODE_DVB_DVR:
> + case MEDIA_ENT_T_DVB_TSOUT:
> dvr = entity;
> break;
> - case MEDIA_ENT_T_DEVNODE_DVB_CA:
> + case MEDIA_ENT_T_DVB_CA:
> ca = entity;
> break;
> }
> }
>
> - if (tuner && fe)
> - media_create_pad_link(tuner, 0, fe, 0, 0);
> + if (tuner && demod)
> + media_create_pad_link(tuner, 0, demod, 0, 0);
>
> - if (fe && demux)
> - media_create_pad_link(fe, 1, demux, 0, MEDIA_LNK_FL_ENABLED);
> + if (demod && demux)
> + media_create_pad_link(demod, 1, demux, 0, MEDIA_LNK_FL_ENABLED);
>
> if (demux && dvr)
> media_create_pad_link(demux, 1, dvr, 0, MEDIA_LNK_FL_ENABLED);
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 21c96cd7a6ae..7306aeaff807 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -42,31 +42,67 @@ struct media_device_info {
>
> #define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
>
> +/*
> + * Base numbers for entity types
> + *
> + * Please notice that the huge gap of 16 bits for each base is overkill!
> + * 8 bits is more than enough to avoid starving entity types for each
> + * subsystem.
> + *
> + * However, It is kept this way just to avoid binary breakages with the
> + * namespace provided on legacy versions of this header.
> + */
> +#define MEDIA_ENT_T_DVB_BASE 0x00000000
> +#define MEDIA_ENT_T_V4L2_BASE 0x00010000
> +#define MEDIA_ENT_T_V4L2_SUBDEV_BASE 0x00020000
> +
> +/* V4L2 entities */
> +#define MEDIA_ENT_T_V4L2_VIDEO (MEDIA_ENT_T_V4L2_BASE + 1)
> + /*
> + * Please notice that numbers between MEDIA_ENT_T_V4L2_BASE + 2 and
> + * MEDIA_ENT_T_V4L2_BASE + 4 can't be used, as those values used
> + * to be declared for FB, ALSA and DVB entities.
> + * As those values were never atually used in practice, we're just
s/atually/actually/
> + * adding them as backward compatibily macros and keeping the
s/compatibily/compatibility/
> + * numberspace cleaned here. This way, we avoid breaking compilation,
s/cleaned/clean/
> + * in the case of having some userspace application using the old
> + * symbols.
> + */
> +#define MEDIA_ENT_T_V4L2_VBI (MEDIA_ENT_T_V4L2_BASE + 5)
> +#define MEDIA_ENT_T_V4L2_RADIO (MEDIA_ENT_T_V4L2_BASE + 6)
> +#define MEDIA_ENT_T_V4L2_SWRADIO (MEDIA_ENT_T_V4L2_BASE + 7)
Why are these entities? Aren't these interface types?
> +
> +/* V4L2 Sub-device entities */
> +#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 1)
> +#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 2)
> +#define MEDIA_ENT_T_V4L2_SUBDEV_LENS (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 3)
> + /* A converter of analogue video to its digital representation. */
> +#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 4)
> + /* Tuner entity is actually both V4L2 and DVB subdev */
> +#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV_BASE + 5)
> +
> +/* DVB entities */
> +#define MEDIA_ENT_T_DVB_DEMOD (MEDIA_ENT_T_DVB_BASE + 4)
> +#define MEDIA_ENT_T_DVB_DEMUX (MEDIA_ENT_T_DVB_BASE + 5)
> +#define MEDIA_ENT_T_DVB_TSOUT (MEDIA_ENT_T_DVB_BASE + 6)
> +#define MEDIA_ENT_T_DVB_CA (MEDIA_ENT_T_DVB_BASE + 7)
> +#define MEDIA_ENT_T_DVB_NET_DECAP (MEDIA_ENT_T_DVB_BASE + 8)
> +
> +/* Legacy symbols used to avoid userspace compilation breakages */
> #define MEDIA_ENT_TYPE_SHIFT 16
> #define MEDIA_ENT_TYPE_MASK 0x00ff0000
> #define MEDIA_ENT_SUBTYPE_MASK 0x0000ffff
>
> -#define MEDIA_ENT_T_DEVNODE (1 << MEDIA_ENT_TYPE_SHIFT)
> -#define MEDIA_ENT_T_DEVNODE_V4L (MEDIA_ENT_T_DEVNODE + 1)
> +#define MEDIA_ENT_T_DEVNODE MEDIA_ENT_T_V4L2_BASE
> +#define MEDIA_ENT_T_V4L2_SUBDEV MEDIA_ENT_T_V4L2_SUBDEV_BASE
> +
> +#define MEDIA_ENT_T_DEVNODE_V4L MEDIA_ENT_T_V4L2_VIDEO
> +
> #define MEDIA_ENT_T_DEVNODE_FB (MEDIA_ENT_T_DEVNODE + 2)
> #define MEDIA_ENT_T_DEVNODE_ALSA (MEDIA_ENT_T_DEVNODE + 3)
> -#define MEDIA_ENT_T_DEVNODE_DVB_FE (MEDIA_ENT_T_DEVNODE + 4)
> -#define MEDIA_ENT_T_DEVNODE_DVB_DEMUX (MEDIA_ENT_T_DEVNODE + 5)
> -#define MEDIA_ENT_T_DEVNODE_DVB_DVR (MEDIA_ENT_T_DEVNODE + 6)
> -#define MEDIA_ENT_T_DEVNODE_DVB_CA (MEDIA_ENT_T_DEVNODE + 7)
> -#define MEDIA_ENT_T_DEVNODE_DVB_NET (MEDIA_ENT_T_DEVNODE + 8)
> +#define MEDIA_ENT_T_DEVNODE_DVB (MEDIA_ENT_T_DEVNODE + 4)
>
> -/* Legacy symbol. Use it to avoid userspace compilation breakages */
> -#define MEDIA_ENT_T_DEVNODE_DVB MEDIA_ENT_T_DEVNODE_DVB_FE
> -
> -#define MEDIA_ENT_T_V4L2_SUBDEV (2 << MEDIA_ENT_TYPE_SHIFT)
> -#define MEDIA_ENT_T_V4L2_SUBDEV_SENSOR (MEDIA_ENT_T_V4L2_SUBDEV + 1)
> -#define MEDIA_ENT_T_V4L2_SUBDEV_FLASH (MEDIA_ENT_T_V4L2_SUBDEV + 2)
> -#define MEDIA_ENT_T_V4L2_SUBDEV_LENS (MEDIA_ENT_T_V4L2_SUBDEV + 3)
> -/* A converter of analogue video to its digital representation. */
> -#define MEDIA_ENT_T_V4L2_SUBDEV_DECODER (MEDIA_ENT_T_V4L2_SUBDEV + 4)
> -
> -#define MEDIA_ENT_T_V4L2_SUBDEV_TUNER (MEDIA_ENT_T_V4L2_SUBDEV + 5)
> +/* Entity types */
>
> #define MEDIA_ENT_FL_DEFAULT (1 << 0)
>
>
Hmm, I'm postponing further review. It might become clearer after reviewing
more of this patch series.
One reason why this is a bit difficult to review is that it is not immediately
obvious which defines are here for backwards compat (and shouldn't be used in
the kernel anymore) and which defines are new.
May I suggest that either in this or in a later patch the defines that shouldn't
be used in the kernel should be placed under #ifndef __KERNEL__?
Regards,
Hans
^ permalink raw reply
* Re: [PATCH v7 13/44] [media] uapi/media.h: Declare interface types
From: Hans Verkuil @ 2015-08-25 6:46 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Linux Media Mailing List
Cc: Mauro Carvalho Chehab, linux-api
In-Reply-To: <55df3b23389e68b19354011babf0da1d26d0a91a.1440359643.git.mchehab@osg.samsung.com>
On 08/23/2015 10:17 PM, Mauro Carvalho Chehab wrote:
> Declare the interface types that will be used by the new
> G_TOPOLOGY ioctl that will be defined latter on.
>
> For now, we need those types, as they'll be used on the
> internal structs associated with the new media_interface
> graph object defined on the next patch.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
>
> diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
> index 4e816be3de39..21c96cd7a6ae 100644
> --- a/include/uapi/linux/media.h
> +++ b/include/uapi/linux/media.h
> @@ -167,6 +167,35 @@ struct media_links_enum {
> __u32 reserved[4];
> };
>
> +/* Interface type ranges */
> +
> +#define MEDIA_INTF_T_DVB_BASE 0x00000000
> +#define MEDIA_INTF_T_V4L_BASE 0x00000100
> +#define MEDIA_INTF_T_ALSA_BASE 0x00000200
I would avoid BASE 0 and start with 0x100 for DVB (so ALSA gets 0x300).
This ensures that type is never 0 which is often useful since it catches
cases where userspace just memsets to 0 and never fills in the type. Or
it can be used in the future as an ERROR or UNKNOWN type or something.
Since there is nothing that requires type to be 0 I would avoid it
altogether.
After making this small change:
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
Regards,
Hans
> +
> +/* Interface types */
> +
> +#define MEDIA_INTF_T_DVB_FE (MEDIA_INTF_T_DVB_BASE)
> +#define MEDIA_INTF_T_DVB_DEMUX (MEDIA_INTF_T_DVB_BASE + 1)
> +#define MEDIA_INTF_T_DVB_DVR (MEDIA_INTF_T_DVB_BASE + 2)
> +#define MEDIA_INTF_T_DVB_CA (MEDIA_INTF_T_DVB_BASE + 3)
> +#define MEDIA_INTF_T_DVB_NET (MEDIA_INTF_T_DVB_BASE + 4)
> +
> +#define MEDIA_INTF_T_V4L_VIDEO (MEDIA_INTF_T_V4L_BASE)
> +#define MEDIA_INTF_T_V4L_VBI (MEDIA_INTF_T_V4L_BASE + 1)
> +#define MEDIA_INTF_T_V4L_RADIO (MEDIA_INTF_T_V4L_BASE + 2)
> +#define MEDIA_INTF_T_V4L_SUBDEV (MEDIA_INTF_T_V4L_BASE + 3)
> +#define MEDIA_INTF_T_V4L_SWRADIO (MEDIA_INTF_T_V4L_BASE + 4)
> +
> +#define MEDIA_INTF_T_ALSA_PCM_CAPTURE (MEDIA_INTF_T_ALSA_BASE)
> +#define MEDIA_INTF_T_ALSA_PCM_PLAYBACK (MEDIA_INTF_T_ALSA_BASE + 1)
> +#define MEDIA_INTF_T_ALSA_CONTROL (MEDIA_INTF_T_ALSA_BASE + 2)
> +#define MEDIA_INTF_T_ALSA_COMPRESS (MEDIA_INTF_T_ALSA_BASE + 3)
> +#define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4)
> +#define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)
> +
> +/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
> +
> #define MEDIA_IOC_DEVICE_INFO _IOWR('|', 0x00, struct media_device_info)
> #define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct media_entity_desc)
> #define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
>
^ permalink raw reply
* Re: [PATCH -mm] selftests/capabilities: Add tests for capability evolution
From: Kees Cook @ 2015-08-24 23:40 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Andrew Morton, Christoph Lameter, James Morris, Andrew Morton,
Serge E. Hallyn, Serge Hallyn, James Morris, Jarkko Sakkinen,
Ted Ts'o, Andrew G. Morgan, Linux API, Mimi Zohar,
Michael Kerrisk, Austin S Hemmelgarn, linux-security-module,
Aaron Jones, Serge Hallyn, LKML, Markku Savela, Jonathan Corbet
In-Reply-To: <2d98ecd81eb807d576dcb4acaa62e70aa5d65e70.1440456948.git.luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Mon, Aug 24, 2015 at 4:03 PM, Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> This test focuses on ambient capabilities. It requires either root
> or the ability to create user namespaces. Some of the test cases
> will be skipped for nonroot users.
>
> Signed-off-by: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Looks great! Thanks for this!
Acked-by: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
-Kees
> ---
>
> I took taking advantage of the extra week to make my test case work :)
>
> tools/testing/selftests/capabilities/.gitignore | 2 +
> tools/testing/selftests/capabilities/Makefile | 19 +
> tools/testing/selftests/capabilities/test_execve.c | 427 +++++++++++++++++++++
> .../testing/selftests/capabilities/validate_cap.c | 73 ++++
> 4 files changed, 521 insertions(+)
> create mode 100644 tools/testing/selftests/capabilities/.gitignore
> create mode 100644 tools/testing/selftests/capabilities/Makefile
> create mode 100644 tools/testing/selftests/capabilities/test_execve.c
> create mode 100644 tools/testing/selftests/capabilities/validate_cap.c
>
> diff --git a/tools/testing/selftests/capabilities/.gitignore b/tools/testing/selftests/capabilities/.gitignore
> new file mode 100644
> index 000000000000..b732dd0d4738
> --- /dev/null
> +++ b/tools/testing/selftests/capabilities/.gitignore
> @@ -0,0 +1,2 @@
> +test_execve
> +validate_cap
> diff --git a/tools/testing/selftests/capabilities/Makefile b/tools/testing/selftests/capabilities/Makefile
> new file mode 100644
> index 000000000000..5b90ed14cccb
> --- /dev/null
> +++ b/tools/testing/selftests/capabilities/Makefile
> @@ -0,0 +1,19 @@
> +all:
> +
> +include ../lib.mk
> +
> +.PHONY: all clean
> +
> +TARGETS := validate_cap test_execve
> +TEST_PROGS := test_execve
> +
> +CFLAGS := -O2 -g -std=gnu99 -Wall -lcap-ng
> +
> +all: $(TARGETS)
> +
> +clean:
> + $(RM) $(TARGETS)
> +
> +$(TARGETS): %: %.c
> + $(CC) -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
> +
> diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
> new file mode 100644
> index 000000000000..10a21a958aaf
> --- /dev/null
> +++ b/tools/testing/selftests/capabilities/test_execve.c
> @@ -0,0 +1,427 @@
> +#define _GNU_SOURCE
> +
> +#include <cap-ng.h>
> +#include <err.h>
> +#include <linux/capability.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <stdarg.h>
> +#include <sched.h>
> +#include <sys/mount.h>
> +#include <limits.h>
> +#include <libgen.h>
> +#include <malloc.h>
> +#include <sys/wait.h>
> +#include <sys/prctl.h>
> +#include <sys/stat.h>
> +
> +#ifndef PR_CAP_AMBIENT
> +#define PR_CAP_AMBIENT 47
> +# define PR_CAP_AMBIENT_IS_SET 1
> +# define PR_CAP_AMBIENT_RAISE 2
> +# define PR_CAP_AMBIENT_LOWER 3
> +# define PR_CAP_AMBIENT_CLEAR_ALL 4
> +#endif
> +
> +static int nerrs;
> +
> +static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list ap)
> +{
> + char buf[4096];
> + int fd;
> + ssize_t written;
> + int buf_len;
> +
> + buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
> + if (buf_len < 0) {
> + err(1, "vsnprintf failed");
> + }
> + if (buf_len >= sizeof(buf)) {
> + errx(1, "vsnprintf output truncated");
> + }
> +
> + fd = open(filename, O_WRONLY);
> + if (fd < 0) {
> + if ((errno == ENOENT) && enoent_ok)
> + return;
> + err(1, "open of %s failed", filename);
> + }
> + written = write(fd, buf, buf_len);
> + if (written != buf_len) {
> + if (written >= 0) {
> + errx(1, "short write to %s", filename);
> + } else {
> + err(1, "write to %s failed", filename);
> + }
> + }
> + if (close(fd) != 0) {
> + err(1, "close of %s failed", filename);
> + }
> +}
> +
> +static void maybe_write_file(char *filename, char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vmaybe_write_file(true, filename, fmt, ap);
> + va_end(ap);
> +}
> +
> +static void write_file(char *filename, char *fmt, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vmaybe_write_file(false, filename, fmt, ap);
> + va_end(ap);
> +}
> +
> +static bool create_and_enter_ns(uid_t inner_uid)
> +{
> + uid_t outer_uid;
> + gid_t outer_gid;
> + int i;
> + bool have_outer_privilege;
> +
> + outer_uid = getuid();
> + outer_gid = getgid();
> +
> + /*
> + * TODO: If we're already root, we could skip creating the userns.
> + */
> +
> + if (unshare(CLONE_NEWNS) == 0) {
> + printf("[NOTE]\tUsing global UIDs for tests\n");
> + if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) != 0)
> + err(1, "PR_SET_KEEPCAPS");
> + if (setresuid(inner_uid, inner_uid, -1) != 0)
> + err(1, "setresuid");
> +
> + // Re-enable effective caps
> + capng_get_caps_process();
> + for (i = 0; i < CAP_LAST_CAP; i++)
> + if (capng_have_capability(CAPNG_PERMITTED, i))
> + capng_update(CAPNG_ADD, CAPNG_EFFECTIVE, i);
> + if (capng_apply(CAPNG_SELECT_CAPS) != 0)
> + err(1, "capng_apply");
> +
> + have_outer_privilege = true;
> + } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
> + printf("[NOTE]\tUsing a user namespace for tests\n");
> + maybe_write_file("/proc/self/setgroups", "deny");
> + write_file("/proc/self/uid_map", "%d %d 1", inner_uid, outer_uid);
> + write_file("/proc/self/gid_map", "0 %d 1", outer_gid);
> +
> + have_outer_privilege = false;
> + } else {
> + errx(1, "must be root or be able to create a userns");
> + }
> +
> + if (mount("none", "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
> + err(1, "remount everything private");
> +
> + return have_outer_privilege;
> +}
> +
> +static void chdir_to_tmpfs(void)
> +{
> + char cwd[PATH_MAX];
> + if (getcwd(cwd, sizeof(cwd)) != cwd)
> + err(1, "getcwd");
> +
> + if (mount("private_tmp", ".", "tmpfs", 0, "mode=0777") != 0)
> + err(1, "mount private tmpfs");
> +
> + if (chdir(cwd) != 0)
> + err(1, "chdir to private tmpfs");
> +
> + if (umount2(".", MNT_DETACH) != 0)
> + err(1, "detach private tmpfs");
> +}
> +
> +static void copy_fromat_to(int fromfd, const char *fromname, const char *toname)
> +{
> + int from = openat(fromfd, fromname, O_RDONLY);
> + if (from == -1)
> + err(1, "open copy source");
> +
> + int to = open(toname, O_CREAT | O_WRONLY | O_EXCL, 0700);
> +
> + while (true) {
> + char buf[4096];
> + ssize_t sz = read(from, buf, sizeof(buf));
> + if (sz == 0)
> + break;
> + if (sz < 0)
> + err(1, "read");
> +
> + if (write(to, buf, sz) != sz)
> + err(1, "write"); /* no short writes on tmpfs */
> + }
> +
> + close(from);
> + close(to);
> +}
> +
> +static bool fork_wait(void)
> +{
> + pid_t child = fork();
> + if (child == 0) {
> + nerrs = 0;
> + return true;
> + } else if (child > 0) {
> + int status;
> + if (waitpid(child, &status, 0) != child ||
> + !WIFEXITED(status)) {
> + printf("[FAIL]\tChild died\n");
> + nerrs++;
> + } else if (WEXITSTATUS(status) != 0) {
> + printf("[FAIL]\tChild failed\n");
> + nerrs++;
> + } else {
> + printf("[OK]\tChild succeeded\n");
> + }
> +
> + return false;
> + } else {
> + err(1, "fork");
> + }
> +}
> +
> +static void exec_other_validate_cap(const char *name,
> + bool eff, bool perm, bool inh, bool ambient)
> +{
> + execl(name, name, (eff ? "1" : "0"),
> + (perm ? "1" : "0"), (inh ? "1" : "0"), (ambient ? "1" : "0"),
> + NULL);
> + err(1, "execl");
> +}
> +
> +static void exec_validate_cap(bool eff, bool perm, bool inh, bool ambient)
> +{
> + exec_other_validate_cap("./validate_cap", eff, perm, inh, ambient);
> +}
> +
> +static int do_tests(int uid, const char *our_path)
> +{
> + bool have_outer_privilege = create_and_enter_ns(uid);
> +
> + int ourpath_fd = open(our_path, O_RDONLY | O_DIRECTORY);
> + if (ourpath_fd == -1)
> + err(1, "open '%s'", our_path);
> +
> + chdir_to_tmpfs();
> +
> + copy_fromat_to(ourpath_fd, "validate_cap", "validate_cap");
> +
> + if (have_outer_privilege) {
> + uid_t gid = getegid();
> +
> + copy_fromat_to(ourpath_fd, "validate_cap",
> + "validate_cap_suidroot");
> + if (chown("validate_cap_suidroot", 0, -1) != 0)
> + err(1, "chown");
> + if (chmod("validate_cap_suidroot", S_ISUID | 0700) != 0)
> + err(1, "chmod");
> +
> + copy_fromat_to(ourpath_fd, "validate_cap",
> + "validate_cap_suidnonroot");
> + if (chown("validate_cap_suidnonroot", uid + 1, -1) != 0)
> + err(1, "chown");
> + if (chmod("validate_cap_suidnonroot", S_ISUID | 0700) != 0)
> + err(1, "chmod");
> +
> + copy_fromat_to(ourpath_fd, "validate_cap",
> + "validate_cap_sgidroot");
> + if (chown("validate_cap_sgidroot", -1, 0) != 0)
> + err(1, "chown");
> + if (chmod("validate_cap_sgidroot", S_ISGID | 0710) != 0)
> + err(1, "chmod");
> +
> + copy_fromat_to(ourpath_fd, "validate_cap",
> + "validate_cap_sgidnonroot");
> + if (chown("validate_cap_sgidnonroot", -1, gid + 1) != 0)
> + err(1, "chown");
> + if (chmod("validate_cap_sgidnonroot", S_ISGID | 0710) != 0)
> + err(1, "chmod");
> +}
> +
> + capng_get_caps_process();
> +
> + /* Make sure that i starts out clear */
> + capng_update(CAPNG_DROP, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
> + if (capng_apply(CAPNG_SELECT_CAPS) != 0)
> + err(1, "capng_apply");
> +
> + if (uid == 0) {
> + printf("[RUN]\tRoot => ep\n");
> + if (fork_wait())
> + exec_validate_cap(true, true, false, false);
> + } else {
> + printf("[RUN]\tNon-root => no caps\n");
> + if (fork_wait())
> + exec_validate_cap(false, false, false, false);
> + }
> +
> + printf("[OK]\tCheck cap_ambient manipulation rules\n");
> +
> + /* We should not be able to add ambient caps yet. */
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != -1 || errno != EPERM) {
> + if (errno == EINVAL)
> + printf("[FAIL]\tPR_CAP_AMBIENT_RAISE isn't supported\n");
> + else
> + printf("[FAIL]\tPR_CAP_AMBIENT_RAISE should have failed eith EPERM on a non-inheritable cap\n");
> + return 1;
> + }
> + printf("[OK]\tPR_CAP_AMBIENT_RAISE failed on non-inheritable cap\n");
> +
> + capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_RAW);
> + capng_update(CAPNG_DROP, CAPNG_PERMITTED, CAP_NET_RAW);
> + capng_update(CAPNG_DROP, CAPNG_EFFECTIVE, CAP_NET_RAW);
> + if (capng_apply(CAPNG_SELECT_CAPS) != 0)
> + err(1, "capng_apply");
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_RAW, 0, 0, 0) != -1 || errno != EPERM) {
> + printf("[FAIL]\tPR_CAP_AMBIENT_RAISE should have failed on a non-permitted cap\n");
> + return 1;
> + }
> + printf("[OK]\tPR_CAP_AMBIENT_RAISE failed on non-permitted cap\n");
> +
> + capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
> + if (capng_apply(CAPNG_SELECT_CAPS) != 0)
> + err(1, "capng_apply");
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
> + printf("[FAIL]\tPR_CAP_AMBIENT_RAISE should have succeeded\n");
> + return 1;
> + }
> + printf("[OK]\tPR_CAP_AMBIENT_RAISE worked\n");
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 1) {
> + printf("[FAIL]\tPR_CAP_AMBIENT_IS_SET is broken\n");
> + return 1;
> + }
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_CLEAR_ALL, 0, 0, 0, 0) != 0)
> + err(1, "PR_CAP_AMBIENT_CLEAR_ALL");
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
> + printf("[FAIL]\tPR_CAP_AMBIENT_CLEAR_ALL didn't work\n");
> + return 1;
> + }
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0)
> + err(1, "PR_CAP_AMBIENT_RAISE");
> +
> + capng_update(CAPNG_DROP, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
> + if (capng_apply(CAPNG_SELECT_CAPS) != 0)
> + err(1, "capng_apply");
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
> + printf("[FAIL]\tDropping I should have dropped A\n");
> + return 1;
> + }
> +
> + printf("[OK]\tBasic manipulation appears to work\n");
> +
> + capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
> + if (capng_apply(CAPNG_SELECT_CAPS) != 0)
> + err(1, "capng_apply");
> + if (uid == 0) {
> + printf("[RUN]\tRoot +i => eip\n");
> + if (fork_wait())
> + exec_validate_cap(true, true, true, false);
> + } else {
> + printf("[RUN]\tNon-root +i => i\n");
> + if (fork_wait())
> + exec_validate_cap(false, false, true, false);
> + }
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0)
> + err(1, "PR_CAP_AMBIENT_RAISE");
> +
> + printf("[RUN]\tUID %d +ia => eipa\n", uid);
> + if (fork_wait())
> + exec_validate_cap(true, true, true, true);
> +
> + /* The remaining tests need real privilege */
> +
> + if (!have_outer_privilege) {
> + printf("[SKIP]\tSUID/SGID tests (needs privilege)\n");
> + goto done;
> + }
> +
> + if (uid == 0) {
> + printf("[RUN]\tRoot +ia, suidroot => eipa\n");
> + if (fork_wait())
> + exec_other_validate_cap("./validate_cap_suidroot",
> + true, true, true, true);
> +
> + printf("[RUN]\tRoot +ia, suidnonroot => ip\n");
> + if (fork_wait())
> + exec_other_validate_cap("./validate_cap_suidnonroot",
> + false, true, true, false);
> +
> + printf("[RUN]\tRoot +ia, sgidroot => eipa\n");
> + if (fork_wait())
> + exec_other_validate_cap("./validate_cap_sgidroot",
> + true, true, true, true);
> +
> + if (fork_wait()) {
> + printf("[RUN]\tRoot, gid != 0, +ia, sgidroot => eip\n");
> + if (setresgid(1, 1, 1) != 0)
> + err(1, "setresgid");
> + exec_other_validate_cap("./validate_cap_sgidroot",
> + true, true, true, false);
> + }
> +
> + printf("[RUN]\tRoot +ia, sgidnonroot => eip\n");
> + if (fork_wait())
> + exec_other_validate_cap("./validate_cap_sgidnonroot",
> + true, true, true, false);
> + } else {
> + printf("[RUN]\tNon-root +ia, sgidnonroot => i\n");
> + exec_other_validate_cap("./validate_cap_sgidnonroot",
> + false, false, true, false);
> +
> + if (fork_wait()) {
> + printf("[RUN]\tNon-root +ia, sgidroot => i\n");
> + if (setresgid(1, 1, 1) != 0)
> + err(1, "setresgid");
> + exec_other_validate_cap("./validate_cap_sgidroot",
> + false, false, true, false);
> + }
> + }
> +
> +done:
> + return nerrs ? 1 : 0;
> +}
> +
> +int main(int argc, char **argv)
> +{
> + char *tmp1, *tmp2, *our_path;
> +
> + /* Find our path */
> + tmp1 = strdup(argv[0]);
> + if (!tmp1)
> + err(1, "strdup");
> + tmp2 = dirname(tmp1);
> + our_path = strdup(tmp2);
> + if (!our_path)
> + err(1, "strdup");
> + free(tmp1);
> +
> + if (fork_wait()) {
> + printf("[RUN]\t+++ Tests with uid == 0 +++\n");
> + return do_tests(0, our_path);
> + }
> +
> + if (fork_wait()) {
> + printf("[RUN]\t+++ Tests with uid != 0 +++\n");
> + return do_tests(1, our_path);
> + }
> +
> + return nerrs ? 1 : 0;
> +}
> diff --git a/tools/testing/selftests/capabilities/validate_cap.c b/tools/testing/selftests/capabilities/validate_cap.c
> new file mode 100644
> index 000000000000..dd3c45f7b23c
> --- /dev/null
> +++ b/tools/testing/selftests/capabilities/validate_cap.c
> @@ -0,0 +1,73 @@
> +#include <cap-ng.h>
> +#include <err.h>
> +#include <linux/capability.h>
> +#include <stdbool.h>
> +#include <string.h>
> +#include <stdio.h>
> +#include <sys/prctl.h>
> +#include <sys/auxv.h>
> +
> +#ifndef PR_CAP_AMBIENT
> +#define PR_CAP_AMBIENT 47
> +# define PR_CAP_AMBIENT_IS_SET 1
> +# define PR_CAP_AMBIENT_RAISE 2
> +# define PR_CAP_AMBIENT_LOWER 3
> +# define PR_CAP_AMBIENT_CLEAR_ALL 4
> +#endif
> +
> +#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 19)
> +# define HAVE_GETAUXVAL
> +#endif
> +
> +static bool bool_arg(char **argv, int i)
> +{
> + if (!strcmp(argv[i], "0"))
> + return false;
> + else if (!strcmp(argv[i], "1"))
> + return true;
> + else
> + errx(1, "wrong argv[%d]", i);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + const char *atsec = "";
> +
> + /*
> + * Be careful just in case a setgid or setcapped copy of this
> + * helper gets out.
> + */
> +
> + if (argc != 5)
> + errx(1, "wrong argc");
> +
> +#ifdef HAVE_GETAUXVAL
> + if (getauxval(AT_SECURE))
> + atsec = " (AT_SECURE is set)";
> + else
> + atsec = " (AT_SECURE is not set)";
> +#endif
> +
> + capng_get_caps_process();
> +
> + if (capng_have_capability(CAPNG_EFFECTIVE, CAP_NET_BIND_SERVICE) != bool_arg(argv, 1)) {
> + printf("[FAIL]\tWrong effective state%s\n", atsec);
> + return 1;
> + }
> + if (capng_have_capability(CAPNG_PERMITTED, CAP_NET_BIND_SERVICE) != bool_arg(argv, 2)) {
> + printf("[FAIL]\tWrong permitted state%s\n", atsec);
> + return 1;
> + }
> + if (capng_have_capability(CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE) != bool_arg(argv, 3)) {
> + printf("[FAIL]\tWrong inheritable state%s\n", atsec);
> + return 1;
> + }
> +
> + if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != bool_arg(argv, 4)) {
> + printf("[FAIL]\tWrong ambient state%s\n", atsec);
> + return 1;
> + }
> +
> + printf("[OK]\tCapabilities after execve were correct\n");
> + return 0;
> +}
> --
> 2.4.3
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* [PATCH -mm] selftests/capabilities: Add tests for capability evolution
From: Andy Lutomirski @ 2015-08-24 23:03 UTC (permalink / raw)
To: Andrew Morton
Cc: Kees Cook, Christoph Lameter, James Morris, Andy Lutomirski,
Andrew Morton, Serge E. Hallyn, Serge Hallyn, James Morris,
Jarkko Sakkinen, Ted Ts'o, Andrew G. Morgan, Linux API,
Mimi Zohar, Michael Kerrisk, Austin S Hemmelgarn,
linux-security-module, Aaron Jones, Serge Hallyn, LKML,
Markku Savela, Jonathan Corbet
This test focuses on ambient capabilities. It requires either root
or the ability to create user namespaces. Some of the test cases
will be skipped for nonroot users.
Signed-off-by: Andy Lutomirski <luto-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
I took taking advantage of the extra week to make my test case work :)
tools/testing/selftests/capabilities/.gitignore | 2 +
tools/testing/selftests/capabilities/Makefile | 19 +
tools/testing/selftests/capabilities/test_execve.c | 427 +++++++++++++++++++++
.../testing/selftests/capabilities/validate_cap.c | 73 ++++
4 files changed, 521 insertions(+)
create mode 100644 tools/testing/selftests/capabilities/.gitignore
create mode 100644 tools/testing/selftests/capabilities/Makefile
create mode 100644 tools/testing/selftests/capabilities/test_execve.c
create mode 100644 tools/testing/selftests/capabilities/validate_cap.c
diff --git a/tools/testing/selftests/capabilities/.gitignore b/tools/testing/selftests/capabilities/.gitignore
new file mode 100644
index 000000000000..b732dd0d4738
--- /dev/null
+++ b/tools/testing/selftests/capabilities/.gitignore
@@ -0,0 +1,2 @@
+test_execve
+validate_cap
diff --git a/tools/testing/selftests/capabilities/Makefile b/tools/testing/selftests/capabilities/Makefile
new file mode 100644
index 000000000000..5b90ed14cccb
--- /dev/null
+++ b/tools/testing/selftests/capabilities/Makefile
@@ -0,0 +1,19 @@
+all:
+
+include ../lib.mk
+
+.PHONY: all clean
+
+TARGETS := validate_cap test_execve
+TEST_PROGS := test_execve
+
+CFLAGS := -O2 -g -std=gnu99 -Wall -lcap-ng
+
+all: $(TARGETS)
+
+clean:
+ $(RM) $(TARGETS)
+
+$(TARGETS): %: %.c
+ $(CC) -o $@ $(CFLAGS) $(EXTRA_CFLAGS) $^ -lrt -ldl
+
diff --git a/tools/testing/selftests/capabilities/test_execve.c b/tools/testing/selftests/capabilities/test_execve.c
new file mode 100644
index 000000000000..10a21a958aaf
--- /dev/null
+++ b/tools/testing/selftests/capabilities/test_execve.c
@@ -0,0 +1,427 @@
+#define _GNU_SOURCE
+
+#include <cap-ng.h>
+#include <err.h>
+#include <linux/capability.h>
+#include <stdbool.h>
+#include <string.h>
+#include <stdio.h>
+#include <fcntl.h>
+#include <errno.h>
+#include <stdarg.h>
+#include <sched.h>
+#include <sys/mount.h>
+#include <limits.h>
+#include <libgen.h>
+#include <malloc.h>
+#include <sys/wait.h>
+#include <sys/prctl.h>
+#include <sys/stat.h>
+
+#ifndef PR_CAP_AMBIENT
+#define PR_CAP_AMBIENT 47
+# define PR_CAP_AMBIENT_IS_SET 1
+# define PR_CAP_AMBIENT_RAISE 2
+# define PR_CAP_AMBIENT_LOWER 3
+# define PR_CAP_AMBIENT_CLEAR_ALL 4
+#endif
+
+static int nerrs;
+
+static void vmaybe_write_file(bool enoent_ok, char *filename, char *fmt, va_list ap)
+{
+ char buf[4096];
+ int fd;
+ ssize_t written;
+ int buf_len;
+
+ buf_len = vsnprintf(buf, sizeof(buf), fmt, ap);
+ if (buf_len < 0) {
+ err(1, "vsnprintf failed");
+ }
+ if (buf_len >= sizeof(buf)) {
+ errx(1, "vsnprintf output truncated");
+ }
+
+ fd = open(filename, O_WRONLY);
+ if (fd < 0) {
+ if ((errno == ENOENT) && enoent_ok)
+ return;
+ err(1, "open of %s failed", filename);
+ }
+ written = write(fd, buf, buf_len);
+ if (written != buf_len) {
+ if (written >= 0) {
+ errx(1, "short write to %s", filename);
+ } else {
+ err(1, "write to %s failed", filename);
+ }
+ }
+ if (close(fd) != 0) {
+ err(1, "close of %s failed", filename);
+ }
+}
+
+static void maybe_write_file(char *filename, char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ vmaybe_write_file(true, filename, fmt, ap);
+ va_end(ap);
+}
+
+static void write_file(char *filename, char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+ vmaybe_write_file(false, filename, fmt, ap);
+ va_end(ap);
+}
+
+static bool create_and_enter_ns(uid_t inner_uid)
+{
+ uid_t outer_uid;
+ gid_t outer_gid;
+ int i;
+ bool have_outer_privilege;
+
+ outer_uid = getuid();
+ outer_gid = getgid();
+
+ /*
+ * TODO: If we're already root, we could skip creating the userns.
+ */
+
+ if (unshare(CLONE_NEWNS) == 0) {
+ printf("[NOTE]\tUsing global UIDs for tests\n");
+ if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) != 0)
+ err(1, "PR_SET_KEEPCAPS");
+ if (setresuid(inner_uid, inner_uid, -1) != 0)
+ err(1, "setresuid");
+
+ // Re-enable effective caps
+ capng_get_caps_process();
+ for (i = 0; i < CAP_LAST_CAP; i++)
+ if (capng_have_capability(CAPNG_PERMITTED, i))
+ capng_update(CAPNG_ADD, CAPNG_EFFECTIVE, i);
+ if (capng_apply(CAPNG_SELECT_CAPS) != 0)
+ err(1, "capng_apply");
+
+ have_outer_privilege = true;
+ } else if (unshare(CLONE_NEWUSER | CLONE_NEWNS) == 0) {
+ printf("[NOTE]\tUsing a user namespace for tests\n");
+ maybe_write_file("/proc/self/setgroups", "deny");
+ write_file("/proc/self/uid_map", "%d %d 1", inner_uid, outer_uid);
+ write_file("/proc/self/gid_map", "0 %d 1", outer_gid);
+
+ have_outer_privilege = false;
+ } else {
+ errx(1, "must be root or be able to create a userns");
+ }
+
+ if (mount("none", "/", NULL, MS_REC | MS_PRIVATE, NULL) != 0)
+ err(1, "remount everything private");
+
+ return have_outer_privilege;
+}
+
+static void chdir_to_tmpfs(void)
+{
+ char cwd[PATH_MAX];
+ if (getcwd(cwd, sizeof(cwd)) != cwd)
+ err(1, "getcwd");
+
+ if (mount("private_tmp", ".", "tmpfs", 0, "mode=0777") != 0)
+ err(1, "mount private tmpfs");
+
+ if (chdir(cwd) != 0)
+ err(1, "chdir to private tmpfs");
+
+ if (umount2(".", MNT_DETACH) != 0)
+ err(1, "detach private tmpfs");
+}
+
+static void copy_fromat_to(int fromfd, const char *fromname, const char *toname)
+{
+ int from = openat(fromfd, fromname, O_RDONLY);
+ if (from == -1)
+ err(1, "open copy source");
+
+ int to = open(toname, O_CREAT | O_WRONLY | O_EXCL, 0700);
+
+ while (true) {
+ char buf[4096];
+ ssize_t sz = read(from, buf, sizeof(buf));
+ if (sz == 0)
+ break;
+ if (sz < 0)
+ err(1, "read");
+
+ if (write(to, buf, sz) != sz)
+ err(1, "write"); /* no short writes on tmpfs */
+ }
+
+ close(from);
+ close(to);
+}
+
+static bool fork_wait(void)
+{
+ pid_t child = fork();
+ if (child == 0) {
+ nerrs = 0;
+ return true;
+ } else if (child > 0) {
+ int status;
+ if (waitpid(child, &status, 0) != child ||
+ !WIFEXITED(status)) {
+ printf("[FAIL]\tChild died\n");
+ nerrs++;
+ } else if (WEXITSTATUS(status) != 0) {
+ printf("[FAIL]\tChild failed\n");
+ nerrs++;
+ } else {
+ printf("[OK]\tChild succeeded\n");
+ }
+
+ return false;
+ } else {
+ err(1, "fork");
+ }
+}
+
+static void exec_other_validate_cap(const char *name,
+ bool eff, bool perm, bool inh, bool ambient)
+{
+ execl(name, name, (eff ? "1" : "0"),
+ (perm ? "1" : "0"), (inh ? "1" : "0"), (ambient ? "1" : "0"),
+ NULL);
+ err(1, "execl");
+}
+
+static void exec_validate_cap(bool eff, bool perm, bool inh, bool ambient)
+{
+ exec_other_validate_cap("./validate_cap", eff, perm, inh, ambient);
+}
+
+static int do_tests(int uid, const char *our_path)
+{
+ bool have_outer_privilege = create_and_enter_ns(uid);
+
+ int ourpath_fd = open(our_path, O_RDONLY | O_DIRECTORY);
+ if (ourpath_fd == -1)
+ err(1, "open '%s'", our_path);
+
+ chdir_to_tmpfs();
+
+ copy_fromat_to(ourpath_fd, "validate_cap", "validate_cap");
+
+ if (have_outer_privilege) {
+ uid_t gid = getegid();
+
+ copy_fromat_to(ourpath_fd, "validate_cap",
+ "validate_cap_suidroot");
+ if (chown("validate_cap_suidroot", 0, -1) != 0)
+ err(1, "chown");
+ if (chmod("validate_cap_suidroot", S_ISUID | 0700) != 0)
+ err(1, "chmod");
+
+ copy_fromat_to(ourpath_fd, "validate_cap",
+ "validate_cap_suidnonroot");
+ if (chown("validate_cap_suidnonroot", uid + 1, -1) != 0)
+ err(1, "chown");
+ if (chmod("validate_cap_suidnonroot", S_ISUID | 0700) != 0)
+ err(1, "chmod");
+
+ copy_fromat_to(ourpath_fd, "validate_cap",
+ "validate_cap_sgidroot");
+ if (chown("validate_cap_sgidroot", -1, 0) != 0)
+ err(1, "chown");
+ if (chmod("validate_cap_sgidroot", S_ISGID | 0710) != 0)
+ err(1, "chmod");
+
+ copy_fromat_to(ourpath_fd, "validate_cap",
+ "validate_cap_sgidnonroot");
+ if (chown("validate_cap_sgidnonroot", -1, gid + 1) != 0)
+ err(1, "chown");
+ if (chmod("validate_cap_sgidnonroot", S_ISGID | 0710) != 0)
+ err(1, "chmod");
+}
+
+ capng_get_caps_process();
+
+ /* Make sure that i starts out clear */
+ capng_update(CAPNG_DROP, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
+ if (capng_apply(CAPNG_SELECT_CAPS) != 0)
+ err(1, "capng_apply");
+
+ if (uid == 0) {
+ printf("[RUN]\tRoot => ep\n");
+ if (fork_wait())
+ exec_validate_cap(true, true, false, false);
+ } else {
+ printf("[RUN]\tNon-root => no caps\n");
+ if (fork_wait())
+ exec_validate_cap(false, false, false, false);
+ }
+
+ printf("[OK]\tCheck cap_ambient manipulation rules\n");
+
+ /* We should not be able to add ambient caps yet. */
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != -1 || errno != EPERM) {
+ if (errno == EINVAL)
+ printf("[FAIL]\tPR_CAP_AMBIENT_RAISE isn't supported\n");
+ else
+ printf("[FAIL]\tPR_CAP_AMBIENT_RAISE should have failed eith EPERM on a non-inheritable cap\n");
+ return 1;
+ }
+ printf("[OK]\tPR_CAP_AMBIENT_RAISE failed on non-inheritable cap\n");
+
+ capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_RAW);
+ capng_update(CAPNG_DROP, CAPNG_PERMITTED, CAP_NET_RAW);
+ capng_update(CAPNG_DROP, CAPNG_EFFECTIVE, CAP_NET_RAW);
+ if (capng_apply(CAPNG_SELECT_CAPS) != 0)
+ err(1, "capng_apply");
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_RAW, 0, 0, 0) != -1 || errno != EPERM) {
+ printf("[FAIL]\tPR_CAP_AMBIENT_RAISE should have failed on a non-permitted cap\n");
+ return 1;
+ }
+ printf("[OK]\tPR_CAP_AMBIENT_RAISE failed on non-permitted cap\n");
+
+ capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
+ if (capng_apply(CAPNG_SELECT_CAPS) != 0)
+ err(1, "capng_apply");
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
+ printf("[FAIL]\tPR_CAP_AMBIENT_RAISE should have succeeded\n");
+ return 1;
+ }
+ printf("[OK]\tPR_CAP_AMBIENT_RAISE worked\n");
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 1) {
+ printf("[FAIL]\tPR_CAP_AMBIENT_IS_SET is broken\n");
+ return 1;
+ }
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_CLEAR_ALL, 0, 0, 0, 0) != 0)
+ err(1, "PR_CAP_AMBIENT_CLEAR_ALL");
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
+ printf("[FAIL]\tPR_CAP_AMBIENT_CLEAR_ALL didn't work\n");
+ return 1;
+ }
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0)
+ err(1, "PR_CAP_AMBIENT_RAISE");
+
+ capng_update(CAPNG_DROP, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
+ if (capng_apply(CAPNG_SELECT_CAPS) != 0)
+ err(1, "capng_apply");
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0) {
+ printf("[FAIL]\tDropping I should have dropped A\n");
+ return 1;
+ }
+
+ printf("[OK]\tBasic manipulation appears to work\n");
+
+ capng_update(CAPNG_ADD, CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE);
+ if (capng_apply(CAPNG_SELECT_CAPS) != 0)
+ err(1, "capng_apply");
+ if (uid == 0) {
+ printf("[RUN]\tRoot +i => eip\n");
+ if (fork_wait())
+ exec_validate_cap(true, true, true, false);
+ } else {
+ printf("[RUN]\tNon-root +i => i\n");
+ if (fork_wait())
+ exec_validate_cap(false, false, true, false);
+ }
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, CAP_NET_BIND_SERVICE, 0, 0, 0) != 0)
+ err(1, "PR_CAP_AMBIENT_RAISE");
+
+ printf("[RUN]\tUID %d +ia => eipa\n", uid);
+ if (fork_wait())
+ exec_validate_cap(true, true, true, true);
+
+ /* The remaining tests need real privilege */
+
+ if (!have_outer_privilege) {
+ printf("[SKIP]\tSUID/SGID tests (needs privilege)\n");
+ goto done;
+ }
+
+ if (uid == 0) {
+ printf("[RUN]\tRoot +ia, suidroot => eipa\n");
+ if (fork_wait())
+ exec_other_validate_cap("./validate_cap_suidroot",
+ true, true, true, true);
+
+ printf("[RUN]\tRoot +ia, suidnonroot => ip\n");
+ if (fork_wait())
+ exec_other_validate_cap("./validate_cap_suidnonroot",
+ false, true, true, false);
+
+ printf("[RUN]\tRoot +ia, sgidroot => eipa\n");
+ if (fork_wait())
+ exec_other_validate_cap("./validate_cap_sgidroot",
+ true, true, true, true);
+
+ if (fork_wait()) {
+ printf("[RUN]\tRoot, gid != 0, +ia, sgidroot => eip\n");
+ if (setresgid(1, 1, 1) != 0)
+ err(1, "setresgid");
+ exec_other_validate_cap("./validate_cap_sgidroot",
+ true, true, true, false);
+ }
+
+ printf("[RUN]\tRoot +ia, sgidnonroot => eip\n");
+ if (fork_wait())
+ exec_other_validate_cap("./validate_cap_sgidnonroot",
+ true, true, true, false);
+ } else {
+ printf("[RUN]\tNon-root +ia, sgidnonroot => i\n");
+ exec_other_validate_cap("./validate_cap_sgidnonroot",
+ false, false, true, false);
+
+ if (fork_wait()) {
+ printf("[RUN]\tNon-root +ia, sgidroot => i\n");
+ if (setresgid(1, 1, 1) != 0)
+ err(1, "setresgid");
+ exec_other_validate_cap("./validate_cap_sgidroot",
+ false, false, true, false);
+ }
+ }
+
+done:
+ return nerrs ? 1 : 0;
+}
+
+int main(int argc, char **argv)
+{
+ char *tmp1, *tmp2, *our_path;
+
+ /* Find our path */
+ tmp1 = strdup(argv[0]);
+ if (!tmp1)
+ err(1, "strdup");
+ tmp2 = dirname(tmp1);
+ our_path = strdup(tmp2);
+ if (!our_path)
+ err(1, "strdup");
+ free(tmp1);
+
+ if (fork_wait()) {
+ printf("[RUN]\t+++ Tests with uid == 0 +++\n");
+ return do_tests(0, our_path);
+ }
+
+ if (fork_wait()) {
+ printf("[RUN]\t+++ Tests with uid != 0 +++\n");
+ return do_tests(1, our_path);
+ }
+
+ return nerrs ? 1 : 0;
+}
diff --git a/tools/testing/selftests/capabilities/validate_cap.c b/tools/testing/selftests/capabilities/validate_cap.c
new file mode 100644
index 000000000000..dd3c45f7b23c
--- /dev/null
+++ b/tools/testing/selftests/capabilities/validate_cap.c
@@ -0,0 +1,73 @@
+#include <cap-ng.h>
+#include <err.h>
+#include <linux/capability.h>
+#include <stdbool.h>
+#include <string.h>
+#include <stdio.h>
+#include <sys/prctl.h>
+#include <sys/auxv.h>
+
+#ifndef PR_CAP_AMBIENT
+#define PR_CAP_AMBIENT 47
+# define PR_CAP_AMBIENT_IS_SET 1
+# define PR_CAP_AMBIENT_RAISE 2
+# define PR_CAP_AMBIENT_LOWER 3
+# define PR_CAP_AMBIENT_CLEAR_ALL 4
+#endif
+
+#if __GLIBC__ > 2 || (__GLIBC__ == 2 && __GLIBC_MINOR__ >= 19)
+# define HAVE_GETAUXVAL
+#endif
+
+static bool bool_arg(char **argv, int i)
+{
+ if (!strcmp(argv[i], "0"))
+ return false;
+ else if (!strcmp(argv[i], "1"))
+ return true;
+ else
+ errx(1, "wrong argv[%d]", i);
+}
+
+int main(int argc, char **argv)
+{
+ const char *atsec = "";
+
+ /*
+ * Be careful just in case a setgid or setcapped copy of this
+ * helper gets out.
+ */
+
+ if (argc != 5)
+ errx(1, "wrong argc");
+
+#ifdef HAVE_GETAUXVAL
+ if (getauxval(AT_SECURE))
+ atsec = " (AT_SECURE is set)";
+ else
+ atsec = " (AT_SECURE is not set)";
+#endif
+
+ capng_get_caps_process();
+
+ if (capng_have_capability(CAPNG_EFFECTIVE, CAP_NET_BIND_SERVICE) != bool_arg(argv, 1)) {
+ printf("[FAIL]\tWrong effective state%s\n", atsec);
+ return 1;
+ }
+ if (capng_have_capability(CAPNG_PERMITTED, CAP_NET_BIND_SERVICE) != bool_arg(argv, 2)) {
+ printf("[FAIL]\tWrong permitted state%s\n", atsec);
+ return 1;
+ }
+ if (capng_have_capability(CAPNG_INHERITABLE, CAP_NET_BIND_SERVICE) != bool_arg(argv, 3)) {
+ printf("[FAIL]\tWrong inheritable state%s\n", atsec);
+ return 1;
+ }
+
+ if (prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, CAP_NET_BIND_SERVICE, 0, 0, 0) != bool_arg(argv, 4)) {
+ printf("[FAIL]\tWrong ambient state%s\n", atsec);
+ return 1;
+ }
+
+ printf("[OK]\tCapabilities after execve were correct\n");
+ return 0;
+}
--
2.4.3
^ permalink raw reply related
* Re: Next round: revised futex(2) man page for review
From: Darren Hart @ 2015-08-24 21:47 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Thomas Gleixner, Torvald Riegel, Carlos O'Donell, Ingo Molnar,
Jakub Jelinek, linux-man, lkml, Davidlohr Bueso, Arnd Bergmann,
Steven Rostedt, Peter Zijlstra, Linux API, Roland McGrath,
Anton Blanchard, Eric Dumazet, bill o gallmeister, Jan Kiszka,
Daniel Wagner, Rich Felker, Andy Lutomirski, bert hubert,
Rusty Russell, Heinrich Schuchardt
In-Reply-To: <55C5A85F.3020202-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Sat, Aug 08, 2015 at 08:57:35AM +0200, Michael Kerrisk (man-pages) wrote:
...
> >> .\" FIXME ===== End of adapted Hart/Guniguntala text =====
> >>
> >>
> >>
> >> .\" FIXME We need some explanation in the following paragraph of *why*
> >> .\" it is important to note that "the kernel will update the
> >> .\" futex word's value prior
> >> It is important to note to returning to user space" . Can someone
> >> explain? that the kernel will update the futex word's value
> >> prior to returning to user space. Unlike the other futex opera‐
> >> tions described above, the PI futex operations are designed for
> >> the implementation of very specific IPC mechanisms.
> >
> > If the kernel didn't perform the update prior to returning to userspace,
> > we could end up in an invalid state. Such as having an owner, but the
> > value being 0. Or having waiters, but not having FUTEX_WAITERS set.
>
> So I've now reworked this passage to read:
>
> It is important to note that the kernel will update the futex
> word's value prior to returning to user space. (This prevents
> the possibility of the futex word's value ending up in an invalid
> state, such as having an owner but the value being 0, or having
> waiters but not having the FUTEX_WAITERS bit set.)
>
> Okay?
Yes.
>
> >> .\"
> >> .\" FIXME XXX In discussing errors for FUTEX_CMP_REQUEUE_PI, Darren Hart
> >> .\" made the observation that "EINVAL is returned if the non-pi
> >> .\" to pi or op pairing semantics are violated."
> >> .\" Probably there needs to be a general statement about this
> >> .\" requirement, probably located at about this point in the page.
> >> .\" Darren (or someone else), care to take a shot at this?
> >
> > We can probably borrow from either the futex.c comments or the
> > futex-requeue-pi.txt in Documentation. Also, it is important to note
> > that the PI requeue operations require two distinct uadders (although
> > that is implied by requiring "non-pi to pi" as a futex cannot be both.
> >
> > Or... perhaps something like:
> >
> > Due to the kernel imposed futex word value policy, PI futex
> > operations have additional usage requirements:
> >
> > FUTEX_WAIT_REQUEUE_PI must be paired with FUTEX_CMP_REQUEUE_PI
> > and be performed from a non-pi futex to a distinct pi futex.
> > Failing to do so will return EINVAL.
>
> For which operation does the EINVAL occur: FUTEX_WAIT_REQUEUE_PI or
> FUTEX_CMP_REQUEUE_PI?
FUTEX_WAIT_REQUEUE_PI can return -EINVAL if called with invalid parameters, such
as uaddr==uaddr2, or (in the case of SHARED futexes), the associated keys match
(meaning it's the same futex word - shared memory, inode, etc.). This can't
happen if the stated policy of requeueing from non-pi to pi is followed as the
same word cannot be both non-pi and pi at the same time, requiring them to be
unique futex words.
FUTEX_CMP_REQUEUE_PI will fail similarly if uaddr and uaddr2 are the same futex
word. Also, if nr_wake != 1.
But, to the point I was making above, FUTEX_CMP_REQUEUE_PI must reque uaddr to
same uaddr2 specified in the previous FUTEX_WAIT_REQUEUE_PI call.
FUTEX_WAIT_REQUEUE_PI sets up the operation, FUTEX_CMP_REQUEUE_PI completes it,
and they must agree on uaddr and uaddr2.
...
> > And their PRIVATE counterparts of course (which is assumed as it is a
> > flag to the opcode).
>
> Yes. But I don't think that needs to be called out explicitly here (?).
Agreed.
>
> >> .\" FIXME XXX ===== Start of adapted Hart/Guniguntala text =====
> >> .\" The following text is drawn from the Hart/Guniguntala paper
> >> .\" (listed in SEE ALSO), but I have reworded some pieces
> >> .\" significantly. Please check it.
> >>
> >> The PI futex operations described below differ from the other
> >> futex operations in that they impose policy on the use of the
> >> value of the futex word:
> >>
> >> * If the lock is not acquired, the futex word's value shall be
> >> 0.
> >>
> >> * If the lock is acquired, the futex word's value shall be the
> >> thread ID (TID; see gettid(2)) of the owning thread.
> >>
> >> * If the lock is owned and there are threads contending for the
> >> lock, then the FUTEX_WAITERS bit shall be set in the futex
> >> word's value; in other words, this value is:
> >>
> >> FUTEX_WAITERS | TID
> >>
> >>
> >> Note that a PI futex word never just has the value FUTEX_WAITERS,
> >> which is a permissible state for non-PI futexes.
> >
> > The second clause is inappropriate. I don't know if that was yours or
> > mine, but non-PI futexes do not have a kernel defined value policy, so
> > ==FUTEX_WAITERS cannot be a "permissible state" as any value is
> > permissible for non-PI futexes, and none have a kernel defined state.
> >
> > Perhaps include a Note under the third bullet as:
> >
> > Note: It is invalid for a PI futex word to have no owner and
> > FUTEX_WAITERS set.
>
> Done.
>
> >> With this policy in place, a user-space application can acquire a
> >> not-acquired lock or release a lock that no other threads try to
> >
> > "that no other threads try to acquire" seems out of place. I think
> > "atomic instructions" is sufficient to express how contention is
> > handled.
>
> Yup, changed.
>
> >> acquire using atomic instructions executed in user space (e.g., a
> >> compare-and-swap operation such as cmpxchg on the x86 architec‐
> >> ture). Acquiring a lock simply consists of using compare-and-
> >> swap to atomically set the futex word's value to the caller's TID
> >> if its previous value was 0. Releasing a lock requires using
> >> compare-and-swap to set the futex word's value to 0 if the previ‐
> >> ous value was the expected TID.
> >>
> >> If a futex is already acquired (i.e., has a nonzero value), wait‐
> >> ers must employ the FUTEX_LOCK_PI operation to acquire the lock.
> >> If other threads are waiting for the lock, then the FUTEX_WAITERS
> >> bit is set in the futex value; in this case, the lock owner must
> >> employ the FUTEX_UNLOCK_PI operation to release the lock.
> >>
> >> In the cases where callers are forced into the kernel (i.e.,
> >> required to perform a futex() call), they then deal directly with
> >> a so-called RT-mutex, a kernel locking mechanism which implements
> >> the required priority-inheritance semantics. After the RT-mutex
> >> is acquired, the futex value is updated accordingly, before the
> >> calling thread returns to user space.
> >
> > This last paragraph relies on kernel implementation rather than
> > behavior. If the RT-mutex is renamed or the mechanism is implemented
> > differently in futexes, this section will require updating. Is that
> > appropriate for a user-space man page?
>
> In the end, I'm not sure. This is (so far) my best attempt at trying
> to convey an explanation of the behavior provided by the API.
>
> results).
>
> But see my question just above. I'll tweak the first bullet point a
> little after I hear back from you.
Arg, lost context. Which question?
In my humble opinion, the paragraph about RT-mutex above should perhaps instead
read something like:
In the cases where callers are forced into the kernel (i.e.,
required to perform a futex() call), they then deal directly with
Linux kernel internal mechanisms which implement the required
priority-inheritance semantics. Once the internal locking structure
is acquired, the futex value is updated accordingly, before the
calling thread returns to user space.
I'm not terribly particular about this, but in my opinion, we should not refer
to internal-only kernel structures in the man page. Feel free to ignore, or to
defer to a differing opinion from Thomas or others.
Thanks for all your work on this!
--
Darren Hart
Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-man" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Eric B Munson @ 2015-08-24 20:26 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Vlastimil Babka, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <CALYGNiO3r9Yx7xeS-rZ_nVCR+BRP4d0-Fnd0omkBDdh1ftnExg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5859 bytes --]
On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
> On Mon, Aug 24, 2015 at 8:00 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
> > On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
> >
> >> On Mon, Aug 24, 2015 at 6:55 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
> >> > On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
> >> >
> >> >> On Mon, Aug 24, 2015 at 6:09 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
> >> >> > On Mon, 24 Aug 2015, Vlastimil Babka wrote:
> >> >> >
> >> >> >> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
> >> >> >> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> >> >> >> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
> >> >> >> >>>>
> >> >> >> >>>>
> >> >> >> >>>>I am in the middle of implementing lock on fault this way, but I cannot
> >> >> >> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
> >> >> >> >>>>the following:
> >> >> >> >>>>
> >> >> >> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
> >> >> >> >>>> mlock(addr, len, MLOCK_ONFAULT);
> >> >> >> >>>> ...
> >> >> >> >>>> mremap(addr, len, 2 * len, ...)
> >> >> >> >>>>
> >> >> >> >>>>There is no way for mremap to know that the area being remapped was lock
> >> >> >> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
> >> >> >> >>>>this without tracking per vma if it was locked with lock or lock on
> >> >> >> >>>>fault?
> >> >> >> >>>
> >> >> >> >>>
> >> >> >> >>>remap can count filled ptes and prefault only completely populated areas.
> >> >> >> >>
> >> >> >> >>
> >> >> >> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
> >> >> >> >>just prepare the page tables and that's it?
> >> >> >> >
> >> >> >> >As I see mremap prefaults pages when it extends mlocked area.
> >> >> >> >
> >> >> >> >Also quote from manpage
> >> >> >> >: If the memory segment specified by old_address and old_size is locked
> >> >> >> >: (using mlock(2) or similar), then this lock is maintained when the segment is
> >> >> >> >: resized and/or relocated. As a consequence, the amount of memory locked
> >> >> >> >: by the process may change.
> >> >> >>
> >> >> >> Oh, right... Well that looks like a convincing argument for having a
> >> >> >> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
> >> >> >> existing pte's would slow it down, and be unreliable (was the area
> >> >> >> completely populated because MLOCK_ONFAULT was not used or because
> >> >> >> the process aulted it already? Was it not populated because
> >> >> >> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
> >> >> >> populate it all?).
> >> >> >
> >> >> > Given this, I am going to stop working in v8 and leave the vma flag in
> >> >> > place.
> >> >> >
> >> >> >>
> >> >> >> The only sane alternative is to populate always for mremap() of
> >> >> >> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
> >> >> >> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
> >> >> >> be enough for Eric's usecase, but it's somewhat ugly.
> >> >> >>
> >> >> >
> >> >> > I don't think that this is the right solution, I would be really
> >> >> > surprised as a user if an area I locked with MLOCK_ONFAULT was then
> >> >> > fully locked and prepopulated after mremap().
> >> >>
> >> >> If mremap is the only problem then we can add opposite flag for it:
> >> >>
> >> >> "MREMAP_NOPOPULATE"
> >> >> - do not populate new segment of locked areas
> >> >> - do not copy normal areas if possible (anonymous/special must be copied)
> >> >>
> >> >> addr = mmap(len, MAP_ANONYMOUS, ...);
> >> >> mlock(addr, len, MLOCK_ONFAULT);
> >> >> ...
> >> >> addr2 = mremap(addr, len, 2 * len, MREMAP_NOPOPULATE);
> >> >> ...
> >> >>
> >> >
> >> > But with this, the user must remember what areas are locked with
> >> > MLOCK_LOCKONFAULT and which are locked the with prepopulate so the
> >> > correct mremap flags can be used.
> >> >
> >>
> >> Yep. Shouldn't be hard. You anyway have to do some changes in user-space.
> >>
> >
> > Sorry if I wasn't clear enough in my last reply, I think forcing
> > userspace to track this is the wrong choice. The VM system is
> > responsible for tracking these attributes and should continue to be.
>
> Userspace tracks addresses and sizes of these areas. Plus mremap obviously
> works only with page granularity so memory allocator in userspace have to know
> a lot about these structures. So keeping one more bit isn't a rocket science.
>
Fair enough, however, my current implementation does not require that
userspace keep track of any extra information. With the VM_LOCKONFAULT
flag mremap() keeps the properties that were set with mlock() or
equivalent across remaps.
> >
> >>
> >> Much simpler for users-pace solution is a mm-wide flag which turns all further
> >> mlocks and MAP_LOCKED into lock-on-fault. Something like
> >> mlockall(MCL_NOPOPULATE_LOCKED).
> >
> > This set certainly adds the foundation for such a change if you think it
> > would be useful. That particular behavior was not part of my inital use
> > case though.
> >
>
> This looks like much easier solution: you don't need new syscall and after
> enabling that lock-on-fault mode userspace still can get old behaviour simply
> by touching newly locked area.
Again, this suggestion requires that userspace know more about VM than
with my implementation and will require it to walk an entire mapping
before use to fault it in if required. With the current implementation,
mlock continues to function as it has, with the additional flexibility
of being able to request that areas not be prepopulated.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Konstantin Khlebnikov @ 2015-08-24 18:53 UTC (permalink / raw)
To: Eric B Munson
Cc: Vlastimil Babka, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <20150824170028.GC17005-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
On Mon, Aug 24, 2015 at 8:00 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
>
>> On Mon, Aug 24, 2015 at 6:55 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
>> >
>> >> On Mon, Aug 24, 2015 at 6:09 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
>> >> > On Mon, 24 Aug 2015, Vlastimil Babka wrote:
>> >> >
>> >> >> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
>> >> >> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org> wrote:
>> >> >> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
>> >> >> >>>>
>> >> >> >>>>
>> >> >> >>>>I am in the middle of implementing lock on fault this way, but I cannot
>> >> >> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
>> >> >> >>>>the following:
>> >> >> >>>>
>> >> >> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> >> >> >>>> mlock(addr, len, MLOCK_ONFAULT);
>> >> >> >>>> ...
>> >> >> >>>> mremap(addr, len, 2 * len, ...)
>> >> >> >>>>
>> >> >> >>>>There is no way for mremap to know that the area being remapped was lock
>> >> >> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
>> >> >> >>>>this without tracking per vma if it was locked with lock or lock on
>> >> >> >>>>fault?
>> >> >> >>>
>> >> >> >>>
>> >> >> >>>remap can count filled ptes and prefault only completely populated areas.
>> >> >> >>
>> >> >> >>
>> >> >> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
>> >> >> >>just prepare the page tables and that's it?
>> >> >> >
>> >> >> >As I see mremap prefaults pages when it extends mlocked area.
>> >> >> >
>> >> >> >Also quote from manpage
>> >> >> >: If the memory segment specified by old_address and old_size is locked
>> >> >> >: (using mlock(2) or similar), then this lock is maintained when the segment is
>> >> >> >: resized and/or relocated. As a consequence, the amount of memory locked
>> >> >> >: by the process may change.
>> >> >>
>> >> >> Oh, right... Well that looks like a convincing argument for having a
>> >> >> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
>> >> >> existing pte's would slow it down, and be unreliable (was the area
>> >> >> completely populated because MLOCK_ONFAULT was not used or because
>> >> >> the process aulted it already? Was it not populated because
>> >> >> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
>> >> >> populate it all?).
>> >> >
>> >> > Given this, I am going to stop working in v8 and leave the vma flag in
>> >> > place.
>> >> >
>> >> >>
>> >> >> The only sane alternative is to populate always for mremap() of
>> >> >> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
>> >> >> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
>> >> >> be enough for Eric's usecase, but it's somewhat ugly.
>> >> >>
>> >> >
>> >> > I don't think that this is the right solution, I would be really
>> >> > surprised as a user if an area I locked with MLOCK_ONFAULT was then
>> >> > fully locked and prepopulated after mremap().
>> >>
>> >> If mremap is the only problem then we can add opposite flag for it:
>> >>
>> >> "MREMAP_NOPOPULATE"
>> >> - do not populate new segment of locked areas
>> >> - do not copy normal areas if possible (anonymous/special must be copied)
>> >>
>> >> addr = mmap(len, MAP_ANONYMOUS, ...);
>> >> mlock(addr, len, MLOCK_ONFAULT);
>> >> ...
>> >> addr2 = mremap(addr, len, 2 * len, MREMAP_NOPOPULATE);
>> >> ...
>> >>
>> >
>> > But with this, the user must remember what areas are locked with
>> > MLOCK_LOCKONFAULT and which are locked the with prepopulate so the
>> > correct mremap flags can be used.
>> >
>>
>> Yep. Shouldn't be hard. You anyway have to do some changes in user-space.
>>
>
> Sorry if I wasn't clear enough in my last reply, I think forcing
> userspace to track this is the wrong choice. The VM system is
> responsible for tracking these attributes and should continue to be.
Userspace tracks addresses and sizes of these areas. Plus mremap obviously
works only with page granularity so memory allocator in userspace have to know
a lot about these structures. So keeping one more bit isn't a rocket science.
>
>>
>> Much simpler for users-pace solution is a mm-wide flag which turns all further
>> mlocks and MAP_LOCKED into lock-on-fault. Something like
>> mlockall(MCL_NOPOPULATE_LOCKED).
>
> This set certainly adds the foundation for such a change if you think it
> would be useful. That particular behavior was not part of my inital use
> case though.
>
This looks like much easier solution: you don't need new syscall and after
enabling that lock-on-fault mode userspace still can get old behaviour simply
by touching newly locked area.
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Eric B Munson @ 2015-08-24 17:00 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Vlastimil Babka, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm@kvack.org, Linux API
In-Reply-To: <CALYGNiPiZgac_TQVuU0907uA6G69wCmV6pBzgpa6sQ-wHLGvGQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 4308 bytes --]
On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
> On Mon, Aug 24, 2015 at 6:55 PM, Eric B Munson <emunson@akamai.com> wrote:
> > On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
> >
> >> On Mon, Aug 24, 2015 at 6:09 PM, Eric B Munson <emunson@akamai.com> wrote:
> >> > On Mon, 24 Aug 2015, Vlastimil Babka wrote:
> >> >
> >> >> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
> >> >> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> >> >> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
> >> >> >>>>
> >> >> >>>>
> >> >> >>>>I am in the middle of implementing lock on fault this way, but I cannot
> >> >> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
> >> >> >>>>the following:
> >> >> >>>>
> >> >> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
> >> >> >>>> mlock(addr, len, MLOCK_ONFAULT);
> >> >> >>>> ...
> >> >> >>>> mremap(addr, len, 2 * len, ...)
> >> >> >>>>
> >> >> >>>>There is no way for mremap to know that the area being remapped was lock
> >> >> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
> >> >> >>>>this without tracking per vma if it was locked with lock or lock on
> >> >> >>>>fault?
> >> >> >>>
> >> >> >>>
> >> >> >>>remap can count filled ptes and prefault only completely populated areas.
> >> >> >>
> >> >> >>
> >> >> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
> >> >> >>just prepare the page tables and that's it?
> >> >> >
> >> >> >As I see mremap prefaults pages when it extends mlocked area.
> >> >> >
> >> >> >Also quote from manpage
> >> >> >: If the memory segment specified by old_address and old_size is locked
> >> >> >: (using mlock(2) or similar), then this lock is maintained when the segment is
> >> >> >: resized and/or relocated. As a consequence, the amount of memory locked
> >> >> >: by the process may change.
> >> >>
> >> >> Oh, right... Well that looks like a convincing argument for having a
> >> >> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
> >> >> existing pte's would slow it down, and be unreliable (was the area
> >> >> completely populated because MLOCK_ONFAULT was not used or because
> >> >> the process aulted it already? Was it not populated because
> >> >> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
> >> >> populate it all?).
> >> >
> >> > Given this, I am going to stop working in v8 and leave the vma flag in
> >> > place.
> >> >
> >> >>
> >> >> The only sane alternative is to populate always for mremap() of
> >> >> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
> >> >> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
> >> >> be enough for Eric's usecase, but it's somewhat ugly.
> >> >>
> >> >
> >> > I don't think that this is the right solution, I would be really
> >> > surprised as a user if an area I locked with MLOCK_ONFAULT was then
> >> > fully locked and prepopulated after mremap().
> >>
> >> If mremap is the only problem then we can add opposite flag for it:
> >>
> >> "MREMAP_NOPOPULATE"
> >> - do not populate new segment of locked areas
> >> - do not copy normal areas if possible (anonymous/special must be copied)
> >>
> >> addr = mmap(len, MAP_ANONYMOUS, ...);
> >> mlock(addr, len, MLOCK_ONFAULT);
> >> ...
> >> addr2 = mremap(addr, len, 2 * len, MREMAP_NOPOPULATE);
> >> ...
> >>
> >
> > But with this, the user must remember what areas are locked with
> > MLOCK_LOCKONFAULT and which are locked the with prepopulate so the
> > correct mremap flags can be used.
> >
>
> Yep. Shouldn't be hard. You anyway have to do some changes in user-space.
>
Sorry if I wasn't clear enough in my last reply, I think forcing
userspace to track this is the wrong choice. The VM system is
responsible for tracking these attributes and should continue to be.
>
> Much simpler for users-pace solution is a mm-wide flag which turns all further
> mlocks and MAP_LOCKED into lock-on-fault. Something like
> mlockall(MCL_NOPOPULATE_LOCKED).
This set certainly adds the foundation for such a change if you think it
would be useful. That particular behavior was not part of my inital use
case though.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Konstantin Khlebnikov @ 2015-08-24 16:22 UTC (permalink / raw)
To: Eric B Munson
Cc: Vlastimil Babka, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <20150824155503.GB17005-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
On Mon, Aug 24, 2015 at 6:55 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
>
>> On Mon, Aug 24, 2015 at 6:09 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
>> > On Mon, 24 Aug 2015, Vlastimil Babka wrote:
>> >
>> >> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
>> >> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org> wrote:
>> >> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
>> >> >>>>
>> >> >>>>
>> >> >>>>I am in the middle of implementing lock on fault this way, but I cannot
>> >> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
>> >> >>>>the following:
>> >> >>>>
>> >> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> >> >>>> mlock(addr, len, MLOCK_ONFAULT);
>> >> >>>> ...
>> >> >>>> mremap(addr, len, 2 * len, ...)
>> >> >>>>
>> >> >>>>There is no way for mremap to know that the area being remapped was lock
>> >> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
>> >> >>>>this without tracking per vma if it was locked with lock or lock on
>> >> >>>>fault?
>> >> >>>
>> >> >>>
>> >> >>>remap can count filled ptes and prefault only completely populated areas.
>> >> >>
>> >> >>
>> >> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
>> >> >>just prepare the page tables and that's it?
>> >> >
>> >> >As I see mremap prefaults pages when it extends mlocked area.
>> >> >
>> >> >Also quote from manpage
>> >> >: If the memory segment specified by old_address and old_size is locked
>> >> >: (using mlock(2) or similar), then this lock is maintained when the segment is
>> >> >: resized and/or relocated. As a consequence, the amount of memory locked
>> >> >: by the process may change.
>> >>
>> >> Oh, right... Well that looks like a convincing argument for having a
>> >> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
>> >> existing pte's would slow it down, and be unreliable (was the area
>> >> completely populated because MLOCK_ONFAULT was not used or because
>> >> the process aulted it already? Was it not populated because
>> >> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
>> >> populate it all?).
>> >
>> > Given this, I am going to stop working in v8 and leave the vma flag in
>> > place.
>> >
>> >>
>> >> The only sane alternative is to populate always for mremap() of
>> >> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
>> >> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
>> >> be enough for Eric's usecase, but it's somewhat ugly.
>> >>
>> >
>> > I don't think that this is the right solution, I would be really
>> > surprised as a user if an area I locked with MLOCK_ONFAULT was then
>> > fully locked and prepopulated after mremap().
>>
>> If mremap is the only problem then we can add opposite flag for it:
>>
>> "MREMAP_NOPOPULATE"
>> - do not populate new segment of locked areas
>> - do not copy normal areas if possible (anonymous/special must be copied)
>>
>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> mlock(addr, len, MLOCK_ONFAULT);
>> ...
>> addr2 = mremap(addr, len, 2 * len, MREMAP_NOPOPULATE);
>> ...
>>
>
> But with this, the user must remember what areas are locked with
> MLOCK_LOCKONFAULT and which are locked the with prepopulate so the
> correct mremap flags can be used.
>
Yep. Shouldn't be hard. You anyway have to do some changes in user-space.
Much simpler for users-pace solution is a mm-wide flag which turns all further
mlocks and MAP_LOCKED into lock-on-fault. Something like
mlockall(MCL_NOPOPULATE_LOCKED).
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Eric B Munson @ 2015-08-24 15:55 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Vlastimil Babka, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm@kvack.org, Linux API
In-Reply-To: <CALYGNiMO+bHCJxqC_f__iS_OgjxTWDUXF4XWVKdS4jGLenWX=g@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3306 bytes --]
On Mon, 24 Aug 2015, Konstantin Khlebnikov wrote:
> On Mon, Aug 24, 2015 at 6:09 PM, Eric B Munson <emunson@akamai.com> wrote:
> > On Mon, 24 Aug 2015, Vlastimil Babka wrote:
> >
> >> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
> >> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> >> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
> >> >>>>
> >> >>>>
> >> >>>>I am in the middle of implementing lock on fault this way, but I cannot
> >> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
> >> >>>>the following:
> >> >>>>
> >> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
> >> >>>> mlock(addr, len, MLOCK_ONFAULT);
> >> >>>> ...
> >> >>>> mremap(addr, len, 2 * len, ...)
> >> >>>>
> >> >>>>There is no way for mremap to know that the area being remapped was lock
> >> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
> >> >>>>this without tracking per vma if it was locked with lock or lock on
> >> >>>>fault?
> >> >>>
> >> >>>
> >> >>>remap can count filled ptes and prefault only completely populated areas.
> >> >>
> >> >>
> >> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
> >> >>just prepare the page tables and that's it?
> >> >
> >> >As I see mremap prefaults pages when it extends mlocked area.
> >> >
> >> >Also quote from manpage
> >> >: If the memory segment specified by old_address and old_size is locked
> >> >: (using mlock(2) or similar), then this lock is maintained when the segment is
> >> >: resized and/or relocated. As a consequence, the amount of memory locked
> >> >: by the process may change.
> >>
> >> Oh, right... Well that looks like a convincing argument for having a
> >> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
> >> existing pte's would slow it down, and be unreliable (was the area
> >> completely populated because MLOCK_ONFAULT was not used or because
> >> the process aulted it already? Was it not populated because
> >> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
> >> populate it all?).
> >
> > Given this, I am going to stop working in v8 and leave the vma flag in
> > place.
> >
> >>
> >> The only sane alternative is to populate always for mremap() of
> >> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
> >> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
> >> be enough for Eric's usecase, but it's somewhat ugly.
> >>
> >
> > I don't think that this is the right solution, I would be really
> > surprised as a user if an area I locked with MLOCK_ONFAULT was then
> > fully locked and prepopulated after mremap().
>
> If mremap is the only problem then we can add opposite flag for it:
>
> "MREMAP_NOPOPULATE"
> - do not populate new segment of locked areas
> - do not copy normal areas if possible (anonymous/special must be copied)
>
> addr = mmap(len, MAP_ANONYMOUS, ...);
> mlock(addr, len, MLOCK_ONFAULT);
> ...
> addr2 = mremap(addr, len, 2 * len, MREMAP_NOPOPULATE);
> ...
>
But with this, the user must remember what areas are locked with
MLOCK_LOCKONFAULT and which are locked the with prepopulate so the
correct mremap flags can be used.
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Konstantin Khlebnikov @ 2015-08-24 15:46 UTC (permalink / raw)
To: Eric B Munson
Cc: Vlastimil Babka, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm@kvack.org, Linux API
In-Reply-To: <20150824150912.GA17005@akamai.com>
On Mon, Aug 24, 2015 at 6:09 PM, Eric B Munson <emunson@akamai.com> wrote:
> On Mon, 24 Aug 2015, Vlastimil Babka wrote:
>
>> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
>> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
>> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
>> >>>>
>> >>>>
>> >>>>I am in the middle of implementing lock on fault this way, but I cannot
>> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
>> >>>>the following:
>> >>>>
>> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> >>>> mlock(addr, len, MLOCK_ONFAULT);
>> >>>> ...
>> >>>> mremap(addr, len, 2 * len, ...)
>> >>>>
>> >>>>There is no way for mremap to know that the area being remapped was lock
>> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
>> >>>>this without tracking per vma if it was locked with lock or lock on
>> >>>>fault?
>> >>>
>> >>>
>> >>>remap can count filled ptes and prefault only completely populated areas.
>> >>
>> >>
>> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
>> >>just prepare the page tables and that's it?
>> >
>> >As I see mremap prefaults pages when it extends mlocked area.
>> >
>> >Also quote from manpage
>> >: If the memory segment specified by old_address and old_size is locked
>> >: (using mlock(2) or similar), then this lock is maintained when the segment is
>> >: resized and/or relocated. As a consequence, the amount of memory locked
>> >: by the process may change.
>>
>> Oh, right... Well that looks like a convincing argument for having a
>> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
>> existing pte's would slow it down, and be unreliable (was the area
>> completely populated because MLOCK_ONFAULT was not used or because
>> the process aulted it already? Was it not populated because
>> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
>> populate it all?).
>
> Given this, I am going to stop working in v8 and leave the vma flag in
> place.
>
>>
>> The only sane alternative is to populate always for mremap() of
>> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
>> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
>> be enough for Eric's usecase, but it's somewhat ugly.
>>
>
> I don't think that this is the right solution, I would be really
> surprised as a user if an area I locked with MLOCK_ONFAULT was then
> fully locked and prepopulated after mremap().
If mremap is the only problem then we can add opposite flag for it:
"MREMAP_NOPOPULATE"
- do not populate new segment of locked areas
- do not copy normal areas if possible (anonymous/special must be copied)
addr = mmap(len, MAP_ANONYMOUS, ...);
mlock(addr, len, MLOCK_ONFAULT);
...
addr2 = mremap(addr, len, 2 * len, MREMAP_NOPOPULATE);
...
>
>> >>
>> >>>There might be a problem after failed populate: remap will handle them
>> >>>as lock on fault. In this case we can fill ptes with swap-like non-present
>> >>>entries to remember that fact and count them as should-be-locked pages.
>> >>
>> >>
>> >>I don't think we should strive to have mremap try to fix the inherent
>> >>unreliability of mmap (MAP_POPULATE)?
>> >
>> >I don't think so. MAP_POPULATE works only when mmap happens.
>> >Flag MREMAP_POPULATE might be a good idea. Just for symmetry.
>>
>> Maybe, but please do it as a separate series.
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org. For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Eric B Munson @ 2015-08-24 15:09 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Konstantin Khlebnikov, Michal Hocko, Andrew Morton,
Jonathan Corbet, Kirill A. Shutemov, Linux Kernel Mailing List,
dri-devel, linux-mm@kvack.org, Linux API
In-Reply-To: <55DB29EB.1000308@suse.cz>
[-- Attachment #1: Type: text/plain, Size: 3250 bytes --]
On Mon, 24 Aug 2015, Vlastimil Babka wrote:
> On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
> >On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka@suse.cz> wrote:
> >>On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
> >>>>
> >>>>
> >>>>I am in the middle of implementing lock on fault this way, but I cannot
> >>>>see how we will hanlde mremap of a lock on fault region. Say we have
> >>>>the following:
> >>>>
> >>>> addr = mmap(len, MAP_ANONYMOUS, ...);
> >>>> mlock(addr, len, MLOCK_ONFAULT);
> >>>> ...
> >>>> mremap(addr, len, 2 * len, ...)
> >>>>
> >>>>There is no way for mremap to know that the area being remapped was lock
> >>>>on fault so it will be locked and prefaulted by remap. How can we avoid
> >>>>this without tracking per vma if it was locked with lock or lock on
> >>>>fault?
> >>>
> >>>
> >>>remap can count filled ptes and prefault only completely populated areas.
> >>
> >>
> >>Does (and should) mremap really prefault non-present pages? Shouldn't it
> >>just prepare the page tables and that's it?
> >
> >As I see mremap prefaults pages when it extends mlocked area.
> >
> >Also quote from manpage
> >: If the memory segment specified by old_address and old_size is locked
> >: (using mlock(2) or similar), then this lock is maintained when the segment is
> >: resized and/or relocated. As a consequence, the amount of memory locked
> >: by the process may change.
>
> Oh, right... Well that looks like a convincing argument for having a
> sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
> existing pte's would slow it down, and be unreliable (was the area
> completely populated because MLOCK_ONFAULT was not used or because
> the process aulted it already? Was it not populated because
> MLOCK_ONFAULT was used, or because mmap(MAP_LOCKED) failed to
> populate it all?).
Given this, I am going to stop working in v8 and leave the vma flag in
place.
>
> The only sane alternative is to populate always for mremap() of
> VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information
> as a limitation of mlock2(MLOCK_ONFAULT). Which might or might not
> be enough for Eric's usecase, but it's somewhat ugly.
>
I don't think that this is the right solution, I would be really
surprised as a user if an area I locked with MLOCK_ONFAULT was then
fully locked and prepopulated after mremap().
> >>
> >>>There might be a problem after failed populate: remap will handle them
> >>>as lock on fault. In this case we can fill ptes with swap-like non-present
> >>>entries to remember that fact and count them as should-be-locked pages.
> >>
> >>
> >>I don't think we should strive to have mremap try to fix the inherent
> >>unreliability of mmap (MAP_POPULATE)?
> >
> >I don't think so. MAP_POPULATE works only when mmap happens.
> >Flag MREMAP_POPULATE might be a good idea. Just for symmetry.
>
> Maybe, but please do it as a separate series.
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Vlastimil Babka @ 2015-08-24 14:27 UTC (permalink / raw)
To: Konstantin Khlebnikov
Cc: Eric B Munson, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <CALYGNiNuZgQFzZ+_dQsPOvSJAX7QfZ38zbabn4wRc=oC5Lb9wA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 08/24/2015 03:50 PM, Konstantin Khlebnikov wrote:
> On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org> wrote:
>> On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
>>>>
>>>>
>>>> I am in the middle of implementing lock on fault this way, but I cannot
>>>> see how we will hanlde mremap of a lock on fault region. Say we have
>>>> the following:
>>>>
>>>> addr = mmap(len, MAP_ANONYMOUS, ...);
>>>> mlock(addr, len, MLOCK_ONFAULT);
>>>> ...
>>>> mremap(addr, len, 2 * len, ...)
>>>>
>>>> There is no way for mremap to know that the area being remapped was lock
>>>> on fault so it will be locked and prefaulted by remap. How can we avoid
>>>> this without tracking per vma if it was locked with lock or lock on
>>>> fault?
>>>
>>>
>>> remap can count filled ptes and prefault only completely populated areas.
>>
>>
>> Does (and should) mremap really prefault non-present pages? Shouldn't it
>> just prepare the page tables and that's it?
>
> As I see mremap prefaults pages when it extends mlocked area.
>
> Also quote from manpage
> : If the memory segment specified by old_address and old_size is locked
> : (using mlock(2) or similar), then this lock is maintained when the segment is
> : resized and/or relocated. As a consequence, the amount of memory locked
> : by the process may change.
Oh, right... Well that looks like a convincing argument for having a
sticky VM_LOCKONFAULT after all. Having mremap guess by scanning
existing pte's would slow it down, and be unreliable (was the area
completely populated because MLOCK_ONFAULT was not used or because the
process aulted it already? Was it not populated because MLOCK_ONFAULT
was used, or because mmap(MAP_LOCKED) failed to populate it all?).
The only sane alternative is to populate always for mremap() of
VM_LOCKED areas, and document this loss of MLOCK_ONFAULT information as
a limitation of mlock2(MLOCK_ONFAULT). Which might or might not be
enough for Eric's usecase, but it's somewhat ugly.
>>
>>> There might be a problem after failed populate: remap will handle them
>>> as lock on fault. In this case we can fill ptes with swap-like non-present
>>> entries to remember that fact and count them as should-be-locked pages.
>>
>>
>> I don't think we should strive to have mremap try to fix the inherent
>> unreliability of mmap (MAP_POPULATE)?
>
> I don't think so. MAP_POPULATE works only when mmap happens.
> Flag MREMAP_POPULATE might be a good idea. Just for symmetry.
Maybe, but please do it as a separate series.
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Konstantin Khlebnikov @ 2015-08-24 13:50 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Eric B Munson, Michal Hocko, Andrew Morton, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <55DB1C77.8070705-AlSwsSmVLrQ@public.gmane.org>
On Mon, Aug 24, 2015 at 4:30 PM, Vlastimil Babka <vbabka-AlSwsSmVLrQ@public.gmane.org> wrote:
> On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
>>>
>>>
>>> I am in the middle of implementing lock on fault this way, but I cannot
>>> see how we will hanlde mremap of a lock on fault region. Say we have
>>> the following:
>>>
>>> addr = mmap(len, MAP_ANONYMOUS, ...);
>>> mlock(addr, len, MLOCK_ONFAULT);
>>> ...
>>> mremap(addr, len, 2 * len, ...)
>>>
>>> There is no way for mremap to know that the area being remapped was lock
>>> on fault so it will be locked and prefaulted by remap. How can we avoid
>>> this without tracking per vma if it was locked with lock or lock on
>>> fault?
>>
>>
>> remap can count filled ptes and prefault only completely populated areas.
>
>
> Does (and should) mremap really prefault non-present pages? Shouldn't it
> just prepare the page tables and that's it?
As I see mremap prefaults pages when it extends mlocked area.
Also quote from manpage
: If the memory segment specified by old_address and old_size is locked
: (using mlock(2) or similar), then this lock is maintained when the segment is
: resized and/or relocated. As a consequence, the amount of memory locked
: by the process may change.
>
>> There might be a problem after failed populate: remap will handle them
>> as lock on fault. In this case we can fill ptes with swap-like non-present
>> entries to remember that fact and count them as should-be-locked pages.
>
>
> I don't think we should strive to have mremap try to fix the inherent
> unreliability of mmap (MAP_POPULATE)?
I don't think so. MAP_POPULATE works only when mmap happens.
Flag MREMAP_POPULATE might be a good idea. Just for symmetry.
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Vlastimil Babka @ 2015-08-24 13:30 UTC (permalink / raw)
To: Konstantin Khlebnikov, Eric B Munson
Cc: Michal Hocko, Andrew Morton, Jonathan Corbet, Kirill A. Shutemov,
Linux Kernel Mailing List, dri-devel, linux-mm@kvack.org,
Linux API
In-Reply-To: <CALYGNiPcruTM+2KKNZr7ebCVCPsqytSrW8rSzSmj+1Qp4OqXEw@mail.gmail.com>
On 08/24/2015 12:17 PM, Konstantin Khlebnikov wrote:
>>
>> I am in the middle of implementing lock on fault this way, but I cannot
>> see how we will hanlde mremap of a lock on fault region. Say we have
>> the following:
>>
>> addr = mmap(len, MAP_ANONYMOUS, ...);
>> mlock(addr, len, MLOCK_ONFAULT);
>> ...
>> mremap(addr, len, 2 * len, ...)
>>
>> There is no way for mremap to know that the area being remapped was lock
>> on fault so it will be locked and prefaulted by remap. How can we avoid
>> this without tracking per vma if it was locked with lock or lock on
>> fault?
>
> remap can count filled ptes and prefault only completely populated areas.
Does (and should) mremap really prefault non-present pages? Shouldn't it
just prepare the page tables and that's it?
> There might be a problem after failed populate: remap will handle them
> as lock on fault. In this case we can fill ptes with swap-like non-present
> entries to remember that fact and count them as should-be-locked pages.
I don't think we should strive to have mremap try to fix the inherent
unreliability of mmap (MAP_POPULATE)?
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v7 3/6] mm: Introduce VM_LOCKONFAULT
From: Konstantin Khlebnikov @ 2015-08-24 10:17 UTC (permalink / raw)
To: Eric B Munson
Cc: Michal Hocko, Andrew Morton, Vlastimil Babka, Jonathan Corbet,
Kirill A. Shutemov, Linux Kernel Mailing List, dri-devel,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, Linux API
In-Reply-To: <20150821183132.GA12835-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
On Fri, Aug 21, 2015 at 9:31 PM, Eric B Munson <emunson-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, 21 Aug 2015, Michal Hocko wrote:
>
>> On Thu 20-08-15 13:03:09, Eric B Munson wrote:
>> > On Thu, 20 Aug 2015, Michal Hocko wrote:
>> >
>> > > On Wed 19-08-15 17:33:45, Eric B Munson wrote:
>> > > [...]
>> > > > The group which asked for this feature here
>> > > > wants the ability to distinguish between LOCKED and LOCKONFAULT regions
>> > > > and without the VMA flag there isn't a way to do that.
>> > >
>> > > Could you be more specific on why this is needed?
>> >
>> > They want to keep metrics on the amount of memory used in a LOCKONFAULT
>> > region versus the address space of the region.
>>
>> /proc/<pid>/smaps already exports that information AFAICS. It exports
>> VMA flags including VM_LOCKED and if rss < size then this is clearly
>> LOCKONFAULT because the standard mlock semantic is to populate. Would
>> that be sufficient?
>>
>> Now, it is true that LOCKONFAULT wouldn't be distinguishable from
>> MAP_LOCKED which failed to populate but does that really matter? It is
>> LOCKONFAULT in a way as well.
>
> Does that matter to my users? No, they do not use MAP_LOCKED at all so
> any VMA with VM_LOCKED set and rss < size is lock on fault. Will it
> matter to others? I suspect so, but these are likely to be the same
> group of users which will be suprised to learn that MAP_LOCKED does not
> guarantee that the entire range is faulted in on return from mmap.
>
>>
>> > > > Do we know that these last two open flags are needed right now or is
>> > > > this speculation that they will be and that none of the other VMA flags
>> > > > can be reclaimed?
>> > >
>> > > I do not think they are needed by anybody right now but that is not a
>> > > reason why it should be used without a really strong justification.
>> > > If the discoverability is really needed then fair enough but I haven't
>> > > seen any justification for that yet.
>> >
>> > To be completely clear you believe that if the metrics collection is
>> > not a strong enough justification, it is better to expand the mm_struct
>> > by another unsigned long than to use one of these bits right?
>>
>> A simple bool is sufficient for that. And yes I think we should go with
>> per mm_struct flag rather than the additional vma flag if it has only
>> the global (whole address space) scope - which would be the case if the
>> LOCKONFAULT is always an mlock modifier and the persistance is needed
>> only for MCL_FUTURE. Which is imho a sane semantic.
>
> I am in the middle of implementing lock on fault this way, but I cannot
> see how we will hanlde mremap of a lock on fault region. Say we have
> the following:
>
> addr = mmap(len, MAP_ANONYMOUS, ...);
> mlock(addr, len, MLOCK_ONFAULT);
> ...
> mremap(addr, len, 2 * len, ...)
>
> There is no way for mremap to know that the area being remapped was lock
> on fault so it will be locked and prefaulted by remap. How can we avoid
> this without tracking per vma if it was locked with lock or lock on
> fault?
remap can count filled ptes and prefault only completely populated areas.
There might be a problem after failed populate: remap will handle them
as lock on fault. In this case we can fill ptes with swap-like non-present
entries to remember that fact and count them as should-be-locked pages.
^ permalink raw reply
* Re: [PATCH v2 0/3] SysFS driver for QEMU fw_cfg device
From: Ard Biesheuvel @ 2015-08-24 7:56 UTC (permalink / raw)
To: Gabriel L. Somlo
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Richard W.M. Jones, Jordan Justen,
x86-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, QEMU Developers,
Gleb Natapov, Matt Fleming,
kernelnewbies-7JyXY6prKcjpASu1u0TL5ti2O/JbrIOy, Gerd Hoffmann,
Paolo Bonzini, Laszlo Ersek,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org,
ralf-6z/3iImG2C8G8FEW9MqTrA, zajec5-Re5JQEeQqe8AvxtiuMwx3w,
paul-DWxLp4Yu+b8AvxtiuMwx3w, Kumar Gala,
linux-api-u79uwXL29TY76Z2rM5mHXA, Leif Lindholm
In-Reply-To: <20150821034757.GA6982-VPZ87SnTp2qKUezXOiBB2eW1CriLhL8O@public.gmane.org>
On 21 August 2015 at 05:47, Gabriel L. Somlo <somlo-D+Gtc/HYRWM@public.gmane.org> wrote:
> On Thu, Aug 20, 2015 at 07:21:48AM +0200, Ard Biesheuvel wrote:
>> On 19 August 2015 at 22:49, Gabriel L. Somlo <somlo-D+Gtc/HYRWM@public.gmane.org> wrote:
>> >> > From: "Gabriel L. Somlo" <somlo-D+Gtc/HYRWM@public.gmane.org>
>> >> >> Several different architectures supported by QEMU are set up with a
>> >> >> "firmware configuration" (fw_cfg) device, used to pass configuration
>> >> >> "blobs" into the guest by the host running QEMU.
>> >> >>
>> >> >> Historically, these config blobs were mostly of interest to the guest
>> >> >> BIOS, but since QEMU v2.4 it is possible to insert arbitrary blobs via
>> >> >> the command line, which makes them potentially interesting to userspace
>> >> >> (e.g. for passing early boot environment variables, etc.).
>> >> >>
>> >> >
>> >> > Does 'potentially interesting' mean you have a use case? Could you elaborate?
>> >
>> > My personal one would be something like:
>> >
>> > cat > guestinfo.txt << EOT
>> > KEY1="val1"
>> > KEY2="val2"
>> > ...
>> > EOT
>> >
>> > qemu-system-x86_64 ... -fw-cfg name="opt/guestinfo",file=./guestinfo.txt ...
>> >
>> > Then, from inside the guest:
>> >
>> > . /sys/firmware/qemu_fw_cfg/by_name/opt/guestinfo/raw
>> >
>> > do_something_with $KEY1 $KEY2
>> > ...
>> >
>> > But I'm thinking this is only one of the many positive things one
>> > could do with the ability to access random host-supplied blobs from
>> > guest userspace :)
>> >
>>
>> 'random host-supplied blobs' sounds awfully like files in a file
>> system to me, and that is already supported by QEMU and works with any
>> guest OS unmodified. If you are in control of the command line, surely
>> you can add a -drive xxx,fat:path/to/blobs -device xxx pair that
>> simply turns up as a volume.
>
> That did come up, here's the start of original thread on the qemu mailing
> list from a while back:
>
> https://lists.gnu.org/archive/html/qemu-devel/2015-02/msg00371.html
>
> To recap, the main advantages to transfering data this way are:
>
> 1. Asynchronous
>
> The host can simply pass data via the qemu command line, and
> not have to care if/when the guest is ready to accept the
> data (i.e. has made it far enough to e.g. start a guest agent)
>
How does that not apply to a file system?
> 2. Out-of-band
>
> I don't have to take over a user-visible element such as a
> disk drive. Same reason VSOCK (or VMWare VMCI for that matter)
> exist and are NOT actual Ethernet/TCP-IP network interfaces :)
>
OK that makes sense. Note that I am not the one you need to convince
that this is a good idea, but I would still like to understand better
why your use case requires this. Could you explain?
--
Ard.
^ permalink raw reply
* [PATCH v7 39/44] [media] uapi/media.h: Add MEDIA_IOC_G_TOPOLOGY ioctl
From: Mauro Carvalho Chehab @ 2015-08-23 20:17 UTC (permalink / raw)
To: Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-api
In-Reply-To: <cover.1440359643.git.mchehab@osg.samsung.com>
Add a new ioctl that will report the entire topology on
one go.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 796e4a490af8..0111d9652b78 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -181,6 +181,8 @@ struct media_interface {
*/
struct media_intf_devnode {
struct media_interface intf;
+
+ /* Should match the fields at media_v2_intf_devnode */
u32 major;
u32 minor;
};
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index ceea791dd6e9..7fcf7f477ae3 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -238,11 +238,94 @@ struct media_links_enum {
#define MEDIA_INTF_T_ALSA_RAWMIDI (MEDIA_INTF_T_ALSA_BASE + 4)
#define MEDIA_INTF_T_ALSA_HWDEP (MEDIA_INTF_T_ALSA_BASE + 5)
-/* TBD: declare the structs needed for the new G_TOPOLOGY ioctl */
+/*
+ * MC next gen API definitions
+ *
+ * NOTE: The declarations below are close to the MC RFC for the Media
+ * Controller, the next generation. Yet, there are a few adjustments
+ * to do, as we want to be able to have a functional API before
+ * the MC properties change. Those will be properly marked below.
+ * Please also notice that I removed "num_pads", "num_links",
+ * from the proposal, as a proper userspace application will likely
+ * use lists for pads/links, just as we intend todo in Kernelspace.
+ * The API definition should be freed from fields that are bound to
+ * some specific data structure.
+ *
+ * FIXME: Currently, I opted to name the new types as "media_v2", as this
+ * won't cause any conflict with the Kernelspace namespace, nor with
+ * the previous kAPI media_*_desc namespace. This can be changed
+ * latter, before the adding this API upstream.
+ */
+
+
+#define MEDIA_NEW_LNK_FL_ENABLED MEDIA_LNK_FL_ENABLED
+#define MEDIA_NEW_LNK_FL_IMMUTABLE MEDIA_LNK_FL_IMMUTABLE
+#define MEDIA_NEW_LNK_FL_DYNAMIC MEDIA_NEW_FL_DYNAMIC
+#define MEDIA_NEW_LNK_FL_INTERFACE_LINK (1 << 3)
+
+struct media_v2_entity {
+ __u32 id;
+ char name[64]; /* FIXME: move to a property? (RFC says so) */
+ __u16 reserved[14];
+};
+
+/* Should match the specific fields at media_intf_devnode */
+struct media_v2_intf_devnode {
+ __u32 major;
+ __u32 minor;
+};
+
+struct media_v2_interface {
+ __u32 id;
+ __u32 intf_type;
+ __u32 flags;
+ __u32 reserved[9];
+
+ union {
+ struct media_v2_intf_devnode devnode;
+ __u32 raw[16];
+ };
+};
+
+struct media_v2_pad {
+ __u32 id;
+ __u32 entity_id;
+ __u32 flags;
+ __u16 reserved[9];
+};
+
+struct media_v2_link {
+ __u32 id;
+ __u32 source_id;
+ __u32 sink_id;
+ __u32 flags;
+ __u32 reserved[5];
+};
+
+struct media_v2_topology {
+ __u32 topology_version;
+
+ __u32 num_entities;
+ struct media_v2_entity *entities;
+
+ __u32 num_interfaces;
+ struct media_v2_interface *interfaces;
+
+ __u32 num_pads;
+ struct media_v2_pad *pads;
+
+ __u32 num_links;
+ struct media_v2_link *links;
+
+ __u32 reserved[64];
+};
+
+/* ioctls */
#define MEDIA_IOC_DEVICE_INFO _IOWR('|', 0x00, struct media_device_info)
#define MEDIA_IOC_ENUM_ENTITIES _IOWR('|', 0x01, struct media_entity_desc)
#define MEDIA_IOC_ENUM_LINKS _IOWR('|', 0x02, struct media_links_enum)
#define MEDIA_IOC_SETUP_LINK _IOWR('|', 0x03, struct media_link_desc)
+#define MEDIA_IOC_G_TOPOLOGY _IOWR('|', 0x04, struct media_v2_topology)
#endif /* __LINUX_MEDIA_H */
--
2.4.3
^ permalink raw reply related
* [PATCH v7 35/44] [media] media controller: get rid of entity subtype on Kernel
From: Mauro Carvalho Chehab @ 2015-08-23 20:17 UTC (permalink / raw)
To: Linux Media Mailing List
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, linux-api
In-Reply-To: <cover.1440359643.git.mchehab@osg.samsung.com>
Don't use anymore the type/subtype entity data/macros
inside the Kernel.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
diff --git a/include/media/media-entity.h b/include/media/media-entity.h
index 952867571429..796e4a490af8 100644
--- a/include/media/media-entity.h
+++ b/include/media/media-entity.h
@@ -185,16 +185,6 @@ struct media_intf_devnode {
u32 minor;
};
-static inline u32 media_entity_type(struct media_entity *entity)
-{
- return entity->type & MEDIA_ENT_TYPE_MASK;
-}
-
-static inline u32 media_entity_subtype(struct media_entity *entity)
-{
- return entity->type & MEDIA_ENT_SUBTYPE_MASK;
-}
-
static inline u32 media_entity_id(struct media_entity *entity)
{
return entity->graph_obj.id;
diff --git a/include/uapi/linux/media.h b/include/uapi/linux/media.h
index e9e7ad268a7e..ceea791dd6e9 100644
--- a/include/uapi/linux/media.h
+++ b/include/uapi/linux/media.h
@@ -42,10 +42,8 @@ struct media_device_info {
#define MEDIA_ENT_ID_FLAG_NEXT (1 << 31)
-/* Used values for media_entity_desc::type */
-
/*
- * Initial value when an entity is created
+ * Initial value to be used when a new entity is created
* Drivers should change it to something useful
*/
#define MEDIA_ENT_T_UNKNOWN 0x00000000
@@ -96,6 +94,7 @@ struct media_device_info {
#define MEDIA_ENT_T_DVB_CA (MEDIA_ENT_T_DVB_BASE + 7)
#define MEDIA_ENT_T_DVB_NET_DECAP (MEDIA_ENT_T_DVB_BASE + 8)
+#ifndef __KERNEL__
/* Legacy symbols used to avoid userspace compilation breakages */
#define MEDIA_ENT_TYPE_SHIFT 16
#define MEDIA_ENT_TYPE_MASK 0x00ff0000
@@ -109,6 +108,7 @@ struct media_device_info {
#define MEDIA_ENT_T_DEVNODE_FB (MEDIA_ENT_T_DEVNODE + 2)
#define MEDIA_ENT_T_DEVNODE_ALSA (MEDIA_ENT_T_DEVNODE + 3)
#define MEDIA_ENT_T_DEVNODE_DVB (MEDIA_ENT_T_DEVNODE + 4)
+#endif
/* Entity types */
--
2.4.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox