Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2] firewire: Remove function callback casts
@ 2020-05-19 17:34 Oscar Carter
  2020-05-20  6:16 ` Takashi Sakamoto
  0 siblings, 1 reply; 6+ messages in thread
From: Oscar Carter @ 2020-05-19 17:34 UTC (permalink / raw)
  To: Kees Cook, Stefan Richter, Mauro Carvalho Chehab,
	Clemens Ladisch, Takashi Sakamoto, Jaroslav Kysela, Takashi Iwai
  Cc: kernel-hardening, linux1394-devel, linux-kernel, linux-media,
	alsa-devel, Oscar Carter, Lev R . Oshvang .

In an effort to enable -Wcast-function-type in the top-level Makefile to
support Control Flow Integrity builds, remove all the function callback
casts.

To do this, modify the "fw_iso_context_create" function prototype adding
a new parameter for the multichannel callback. Also, fix all the
function calls accordingly.

In the "fw_iso_context_create" function return an error code if both
callback parameters are NULL and also set the "ctx->callback.sc"
explicity to NULL in this case. It is not necessary set to NULL the
"ctx->callback.mc" variable because this and "ctx->callback.sc" are an
union and setting one implies setting the other one to the same value.

Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
---
Changelog v1->v2
-Set explicity to NULL the "ctx->callback.sc" variable and return an error
 code in "fw_iso_context_create" function if both callback parameters are
 NULL as Lev R. Oshvang suggested.
-Modify the commit changelog accordingly.

 drivers/firewire/core-cdev.c        | 12 +++++++-----
 drivers/firewire/core-iso.c         | 14 ++++++++++++--
 drivers/firewire/net.c              |  2 +-
 drivers/media/firewire/firedtv-fw.c |  3 ++-
 include/linux/firewire.h            |  3 ++-
 sound/firewire/amdtp-stream.c       |  2 +-
 sound/firewire/isight.c             |  4 ++--
 7 files changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c
index 6e291d8f3a27..cc368b35be2e 100644
--- a/drivers/firewire/core-cdev.c
+++ b/drivers/firewire/core-cdev.c
@@ -957,7 +957,8 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 {
 	struct fw_cdev_create_iso_context *a = &arg->create_iso_context;
 	struct fw_iso_context *context;
-	fw_iso_callback_t cb;
+	fw_iso_callback_t cb_sc = NULL;
+	fw_iso_mc_callback_t cb_mc = NULL;
 	int ret;

 	BUILD_BUG_ON(FW_CDEV_ISO_CONTEXT_TRANSMIT != FW_ISO_CONTEXT_TRANSMIT ||
@@ -970,7 +971,7 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 		if (a->speed > SCODE_3200 || a->channel > 63)
 			return -EINVAL;

-		cb = iso_callback;
+		cb_sc = iso_callback;
 		break;

 	case FW_ISO_CONTEXT_RECEIVE:
@@ -978,11 +979,11 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 		    a->channel > 63)
 			return -EINVAL;

-		cb = iso_callback;
+		cb_sc = iso_callback;
 		break;

 	case FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL:
-		cb = (fw_iso_callback_t)iso_mc_callback;
+		cb_mc = iso_mc_callback;
 		break;

 	default:
@@ -990,7 +991,8 @@ static int ioctl_create_iso_context(struct client *client, union ioctl_arg *arg)
 	}

 	context = fw_iso_context_create(client->device->card, a->type,
-			a->channel, a->speed, a->header_size, cb, client);
+			a->channel, a->speed, a->header_size, cb_sc, cb_mc,
+			client);
 	if (IS_ERR(context))
 		return PTR_ERR(context);
 	if (client->version < FW_CDEV_VERSION_AUTO_FLUSH_ISO_OVERFLOW)
diff --git a/drivers/firewire/core-iso.c b/drivers/firewire/core-iso.c
index 185b0b78b3d6..1d28a98c08b2 100644
--- a/drivers/firewire/core-iso.c
+++ b/drivers/firewire/core-iso.c
@@ -131,7 +131,8 @@ size_t fw_iso_buffer_lookup(struct fw_iso_buffer *buffer, dma_addr_t completed)

 struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
 		int type, int channel, int speed, size_t header_size,
-		fw_iso_callback_t callback, void *callback_data)
+		fw_iso_callback_t cb_sc, fw_iso_mc_callback_t cb_mc,
+		void *callback_data)
 {
 	struct fw_iso_context *ctx;

@@ -145,7 +146,16 @@ struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
 	ctx->channel = channel;
 	ctx->speed = speed;
 	ctx->header_size = header_size;
-	ctx->callback.sc = callback;
+
+	if (cb_sc) {
+		ctx->callback.sc = cb_sc;
+	} else if (cb_mc) {
+		ctx->callback.mc = cb_mc;
+	} else {
+		ctx->callback.sc = NULL;
+		return ERR_PTR(-EINVAL);
+	}
+
 	ctx->callback_data = callback_data;

 	return ctx;
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 715e491dfbc3..c5cc0a311aa0 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -1136,7 +1136,7 @@ static int fwnet_broadcast_start(struct fwnet_device *dev)
 	context = fw_iso_context_create(dev->card, FW_ISO_CONTEXT_RECEIVE,
 					IEEE1394_BROADCAST_CHANNEL,
 					dev->card->link_speed, 8,
-					fwnet_receive_broadcast, dev);
+					fwnet_receive_broadcast, NULL, dev);
 	if (IS_ERR(context)) {
 		retval = PTR_ERR(context);
 		goto failed;
diff --git a/drivers/media/firewire/firedtv-fw.c b/drivers/media/firewire/firedtv-fw.c
index 97144734eb05..d2940adefd8c 100644
--- a/drivers/media/firewire/firedtv-fw.c
+++ b/drivers/media/firewire/firedtv-fw.c
@@ -141,7 +141,8 @@ int fdtv_start_iso(struct firedtv *fdtv)

 	ctx->context = fw_iso_context_create(device->card,
 			FW_ISO_CONTEXT_RECEIVE, fdtv->isochannel,
-			device->max_speed, ISO_HEADER_SIZE, handle_iso, fdtv);
+			device->max_speed, ISO_HEADER_SIZE,
+			handle_iso, NULL, fdtv);
 	if (IS_ERR(ctx->context)) {
 		err = PTR_ERR(ctx->context);
 		goto fail_free;
diff --git a/include/linux/firewire.h b/include/linux/firewire.h
index aec8f30ab200..3a0b5e18e140 100644
--- a/include/linux/firewire.h
+++ b/include/linux/firewire.h
@@ -452,7 +452,8 @@ struct fw_iso_context {

 struct fw_iso_context *fw_iso_context_create(struct fw_card *card,
 		int type, int channel, int speed, size_t header_size,
-		fw_iso_callback_t callback, void *callback_data);
+		fw_iso_callback_t cb_sc, fw_iso_mc_callback_t cb_mc,
+		void *callback_data);
 int fw_iso_context_set_channels(struct fw_iso_context *ctx, u64 *channels);
 int fw_iso_context_queue(struct fw_iso_context *ctx,
 			 struct fw_iso_packet *packet,
diff --git a/sound/firewire/amdtp-stream.c b/sound/firewire/amdtp-stream.c
index 37d38efb4c87..8629ab3e2c64 100644
--- a/sound/firewire/amdtp-stream.c
+++ b/sound/firewire/amdtp-stream.c
@@ -1093,7 +1093,7 @@ static int amdtp_stream_start(struct amdtp_stream *s, int channel, int speed,

 	s->context = fw_iso_context_create(fw_parent_device(s->unit)->card,
 					  type, channel, speed, ctx_header_size,
-					  ctx_cb, ctx_data);
+					  ctx_cb, NULL, ctx_data);
 	if (IS_ERR(s->context)) {
 		err = PTR_ERR(s->context);
 		if (err == -EBUSY)
diff --git a/sound/firewire/isight.c b/sound/firewire/isight.c
index 6655af53b367..51cc37fca736 100644
--- a/sound/firewire/isight.c
+++ b/sound/firewire/isight.c
@@ -361,8 +361,8 @@ static int isight_start_streaming(struct isight *isight)
 	isight->context = fw_iso_context_create(isight->device->card,
 						FW_ISO_CONTEXT_RECEIVE,
 						isight->resources.channel,
-						isight->device->max_speed,
-						4, isight_packet, isight);
+						isight->device->max_speed, 4,
+						isight_packet, NULL, isight);
 	if (IS_ERR(isight->context)) {
 		err = PTR_ERR(isight->context);
 		isight->context = NULL;
--
2.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] firewire: Remove function callback casts
  2020-05-19 17:34 [PATCH v2] firewire: Remove function callback casts Oscar Carter
@ 2020-05-20  6:16 ` Takashi Sakamoto
  2020-05-22 17:43   ` Oscar Carter
  0 siblings, 1 reply; 6+ messages in thread
From: Takashi Sakamoto @ 2020-05-20  6:16 UTC (permalink / raw)
  To: Oscar Carter
  Cc: Kees Cook, Stefan Richter, Mauro Carvalho Chehab,
	Clemens Ladisch, Jaroslav Kysela, Takashi Iwai, kernel-hardening,
	linux1394-devel, linux-kernel, linux-media, alsa-devel,
	Lev R . Oshvang .

Hi,

On Tue, May 19, 2020 at 07:34:25PM +0200, Oscar Carter wrote:
> In an effort to enable -Wcast-function-type in the top-level Makefile to
> support Control Flow Integrity builds, remove all the function callback
> casts.
> 
> To do this, modify the "fw_iso_context_create" function prototype adding
> a new parameter for the multichannel callback. Also, fix all the
> function calls accordingly.
> 
> In the "fw_iso_context_create" function return an error code if both
> callback parameters are NULL and also set the "ctx->callback.sc"
> explicity to NULL in this case. It is not necessary set to NULL the
> "ctx->callback.mc" variable because this and "ctx->callback.sc" are an
> union and setting one implies setting the other one to the same value.
> 
> Signed-off-by: Oscar Carter <oscar.carter@gmx.com>
> ---
> Changelog v1->v2
> -Set explicity to NULL the "ctx->callback.sc" variable and return an error
>  code in "fw_iso_context_create" function if both callback parameters are
>  NULL as Lev R. Oshvang suggested.
> -Modify the commit changelog accordingly.
> 
>  drivers/firewire/core-cdev.c        | 12 +++++++-----
>  drivers/firewire/core-iso.c         | 14 ++++++++++++--
>  drivers/firewire/net.c              |  2 +-
>  drivers/media/firewire/firedtv-fw.c |  3 ++-
>  include/linux/firewire.h            |  3 ++-
>  sound/firewire/amdtp-stream.c       |  2 +-
>  sound/firewire/isight.c             |  4 ++--
>  7 files changed, 27 insertions(+), 13 deletions(-)

I'm an author of ALSA firewire stack and thanks for the patch. I agree with
your intention to remove the cast of function callback toward CFI build.

Practically, the isochronous context with FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL
is never used by in-kernel drivers. Here, I propose to leave current
kernel API (fw_iso_context_create() with fw_iso_callback_t) as is.
Alternatively, a new kernel API for the context (e.g.
fw_iso_mc_context_create() with fw_iso_mc_callback_t). This idea leaves
current drivers as is and the change is done inner firewire-core driver,
therefore existent kernel API is not changed.

Later I post two patches for the proposal. I'd like you to review it and
I'm glad to receive your comments.


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] firewire: Remove function callback casts
  2020-05-20  6:16 ` Takashi Sakamoto
@ 2020-05-22 17:43   ` Oscar Carter
  2020-05-23  6:10     ` greg
  2020-05-23  6:10     ` Greg KH
  0 siblings, 2 replies; 6+ messages in thread
From: Oscar Carter @ 2020-05-22 17:43 UTC (permalink / raw)
  To: Takashi Sakamoto
  Cc: Oscar Carter, Kees Cook, Stefan Richter, Mauro Carvalho Chehab,
	Clemens Ladisch, Jaroslav Kysela, Takashi Iwai, kernel-hardening,
	linux1394-devel, linux-kernel, linux-media, alsa-devel,
	Lev R . Oshvang .

Hi,

On Wed, May 20, 2020 at 03:16:24PM +0900, Takashi Sakamoto wrote:
> Hi,
>
> I'm an author of ALSA firewire stack and thanks for the patch. I agree with
> your intention to remove the cast of function callback toward CFI build.
>
> Practically, the isochronous context with FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL
> is never used by in-kernel drivers. Here, I propose to leave current
> kernel API (fw_iso_context_create() with fw_iso_callback_t) as is.
> Alternatively, a new kernel API for the context (e.g.
> fw_iso_mc_context_create() with fw_iso_mc_callback_t). This idea leaves
> current drivers as is and the change is done inner firewire-core driver,
> therefore existent kernel API is not changed.
>
It sounds good to me.

> Later I post two patches for the proposal. I'd like you to review it and
> I'm glad to receive your comments.
>
I will take a look at your proposal. Thanks for your time and work.
>
> Regards
>
> Takashi Sakamoto

Thanks,
Oscar Carter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] firewire: Remove function callback casts
  2020-05-22 17:43   ` Oscar Carter
@ 2020-05-23  6:10     ` greg
  2020-05-23  6:10     ` Greg KH
  1 sibling, 0 replies; 6+ messages in thread
From: greg @ 2020-05-23  6:10 UTC (permalink / raw)
  To: Oscar Carter
  Cc: stable, Takashi Sakamoto, Kees Cook, Stefan Richter,
	Mauro Carvalho Chehab, Clemens Ladisch, Jaroslav Kysela,
	Takashi Iwai, kernel-hardening, linux1394-devel, linux-kernel,
	linux-media, alsa-devel, Lev R . Oshvang .

On Fri, May 22, 2020 at 07:43:08PM +0200, Oscar Carter wrote:
> Hi,
> 
> On Wed, May 20, 2020 at 03:16:24PM +0900, Takashi Sakamoto wrote:
> > Hi,
> >
> > I'm an author of ALSA firewire stack and thanks for the patch. I agree with
> > your intention to remove the cast of function callback toward CFI build.
> >
> > Practically, the isochronous context with FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL
> > is never used by in-kernel drivers. Here, I propose to leave current
> > kernel API (fw_iso_context_create() with fw_iso_callback_t) as is.

If it's not used by anyone, why is it still there?  Can't we just delete
it?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] firewire: Remove function callback casts
  2020-05-22 17:43   ` Oscar Carter
  2020-05-23  6:10     ` greg
@ 2020-05-23  6:10     ` Greg KH
  2020-05-23  7:58       ` Takashi Sakamoto
  1 sibling, 1 reply; 6+ messages in thread
From: Greg KH @ 2020-05-23  6:10 UTC (permalink / raw)
  To: Oscar Carter
  Cc: stable, Takashi Sakamoto, Kees Cook, Stefan Richter,
	Mauro Carvalho Chehab, Clemens Ladisch, Jaroslav Kysela,
	Takashi Iwai, kernel-hardening, linux1394-devel, linux-kernel,
	linux-media, alsa-devel, Lev R . Oshvang .

On Fri, May 22, 2020 at 07:43:08PM +0200, Oscar Carter wrote:
> Hi,
> 
> On Wed, May 20, 2020 at 03:16:24PM +0900, Takashi Sakamoto wrote:
> > Hi,
> >
> > I'm an author of ALSA firewire stack and thanks for the patch. I agree with
> > your intention to remove the cast of function callback toward CFI build.
> >
> > Practically, the isochronous context with FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL
> > is never used by in-kernel drivers. Here, I propose to leave current
> > kernel API (fw_iso_context_create() with fw_iso_callback_t) as is.

If it's not used by anyone, why is it still there?  Can't we just delete
it?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] firewire: Remove function callback casts
  2020-05-23  6:10     ` Greg KH
@ 2020-05-23  7:58       ` Takashi Sakamoto
  0 siblings, 0 replies; 6+ messages in thread
From: Takashi Sakamoto @ 2020-05-23  7:58 UTC (permalink / raw)
  To: Greg KH
  Cc: Oscar Carter, stable, Kees Cook, Stefan Richter,
	Mauro Carvalho Chehab, Clemens Ladisch, Jaroslav Kysela,
	Takashi Iwai, kernel-hardening, linux1394-devel, linux-kernel,
	linux-media, alsa-devel, Lev R . Oshvang .

Hi Greg,

On Sat, May 23, 2020 at 08:10:33AM +0200, Greg KH wrote:
> On Fri, May 22, 2020 at 07:43:08PM +0200, Oscar Carter wrote:
> > Hi,
> > 
> > On Wed, May 20, 2020 at 03:16:24PM +0900, Takashi Sakamoto wrote:
> > > Hi,
> > >
> > > I'm an author of ALSA firewire stack and thanks for the patch. I agree with
> > > your intention to remove the cast of function callback toward CFI build.
> > >
> > > Practically, the isochronous context with FW_ISO_CONTEXT_RECEIVE_MULTICHANNEL
> > > is never used by in-kernel drivers. Here, I propose to leave current
> > > kernel API (fw_iso_context_create() with fw_iso_callback_t) as is.
> 
> If it's not used by anyone, why is it still there?  Can't we just delete
> it?

For this patchset, I followed to the theory to keep backward compatibility
when adding any change, and it's what I'd like to discuss.

The isoc context of multichannel mode is also available for userspace
applications, and libhinoko[1] uses it. In a point of backward
compatibility for userspace, we can't delete the mode.

(Practically, the mode is useful for the purpose of packet sniffing in
bus and helpful to my work for development of ALSA firewire stack[2].)

On the other hand, there's no unit driver to use the mode in upstream
kernel. It's unlikely to use the mode for unit driver since the mode is
not specific to unit functionality. In my opinion, it's reasonable to
lose backward compatibility slightly by hiding the multichannel mode
from in-kernel unit driver.

I'll post v2 patchset later and I'd like you to merge them if no
objections from the others for the loss of backward compatibility.

[1] https://github.com/takaswie/libhinoko
[2] https://mailman.alsa-project.org/pipermail/alsa-devel/2019-April/147862.html


Regards

Takashi Sakamoto

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 17:34 [PATCH v2] firewire: Remove function callback casts Oscar Carter
2020-05-20  6:16 ` Takashi Sakamoto
2020-05-22 17:43   ` Oscar Carter
2020-05-23  6:10     ` greg
2020-05-23  6:10     ` Greg KH
2020-05-23  7:58       ` Takashi Sakamoto

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org
	public-inbox-index linux-media

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git